Use FSEvents for recursive file watch on Mac

Use FSEvents for recursive file watch on Mac

Brings back the old FSEvents code, changing it to support recursive watches only, and use dispatch queues instead of using the CFRunLoop from the UI thread.

BUG=144491
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276289

[vandebo (ex-Chrome), 2014-06-04 16:06:53]

Resubmit of https://codereview.chromium.org/283423003/ with recursive watcher
tests disabled on 10.6 (because it's not supported).

[Nico, 2014-06-04 16:16:39]

I think Mark and Jeremy are familiar with the FSEvent code (?)

[vandebo (ex-Chrome), 2014-06-05 17:05:54]

On 2014/06/04 16:16:39, Nico (away) wrote:
> I think Mark and Jeremy are familiar with the FSEvent code (?)

Ping - the FSEvents change was already reviewed by mnissler (OOO for summer), I
just need the PS1 -> PS3 delta reviewed (other comments are of course welcome).

[jeremy, 2014-06-05 17:48:44]

So just changes to base/files/file_path_watcher_browsertest.cc ?

It's a weekend here, I'll be happy to take a look at the start of next week
if no-one gets to this first.

Any chance you could upload a diff between this and the old FSEvent code,
just looking at the whole thing makes this kind of hard to review :(


On Thu, Jun 5, 2014 at 8:05 PM, <vandebo@chromium.org> wrote:

> On 2014/06/04 16:16:39, Nico (away) wrote:
>
>> I think Mark and Jeremy are familiar with the FSEvent code (?)
>>
>
> Ping - the FSEvents change was already reviewed by mnissler (OOO for
> summer), I
> just need the PS1 -> PS3 delta reviewed (other comments are of course
> welcome).
>
> https://codereview.chromium.org/313083007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[vandebo (ex-Chrome), 2014-06-05 17:52:52]

On 2014/06/05 17:48:44, jeremy wrote:
> So just changes to base/files/file_path_watcher_browsertest.cc ?

Yup.

> It's a weekend here, I'll be happy to take a look at the start of next week
> if no-one gets to this first.
> 
> Any chance you could upload a diff between this and the old FSEvent code,
> just looking at the whole thing makes this kind of hard to review :(

I'll work on that.  Not sure if the diff will be useful, but it's worth a shot.


 
> On Thu, Jun 5, 2014 at 8:05 PM, <mailto:vandebo@chromium.org> wrote:
> 
> > On 2014/06/04 16:16:39, Nico (away) wrote:
> >
> >> I think Mark and Jeremy are familiar with the FSEvent code (?)
> >>
> >
> > Ping - the FSEvents change was already reviewed by mnissler (OOO for
> > summer), I
> > just need the PS1 -> PS3 delta reviewed (other comments are of course
> > welcome).
> >
> > https://codereview.chromium.org/313083007/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[vandebo (ex-Chrome), 2014-06-05 19:49:26]

On 2014/06/05 17:52:52, vandebo wrote:
> On 2014/06/05 17:48:44, jeremy wrote:
> > Any chance you could upload a diff between this and the old FSEvent code,
> > just looking at the whole thing makes this kind of hard to review :(
> 
> I'll work on that.  Not sure if the diff will be useful, but it's worth a
shot.

Patch set 3 of https://codereview.chromium.org/312333003/ should be a littler
easier to read.

[Mark Mentovai, 2014-06-08 15:29:02]

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:27: :
mac::LibDispatchTaskRunner("chromium.org.FilePathWatcherFSEvents") {
These should be reverse DNS: org.chromium.whatever.

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:27: :
mac::LibDispatchTaskRunner("chromium.org.FilePathWatcherFSEvents") {
Do we really need another thread for this? The UI thread should be capable of
receiving FSEvent notifications. This shouldn’t be considered dangerous because
there won’t be any blocking involved. The UI thread can post tasks to the IO
thread or wherever you actually want things to be handled (as they would be on
other platforms).

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:38: FilePath ResolvePath(const
FilePath& path) {
Can you just call realpath()?

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:111:
watcher->OnFilePathsChanged(paths);
UpdateEventStream happens in the future. OnFilePathsChanged happens right here,
right now. If some changes to the filesystem occur in between these two events,
they’ll be missed.

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.h (right):

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:19: // implementation. The biggest
issues are that FSEvents on 10.6 sometimes drops
FSEvents can drop events on any OS. Who are the callers and what are their
requirements? Who can tolerate flakiness? Can anyone not tolerate flakiness? Who
needs file-modify events?

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
File base/files/file_path_watcher_mac.cc (right):

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_mac.cc:27: #if !defined(OS_IOS)
file_path_watcher_browsertest has a function RecursiveWatchAvaiable that
basically duplicates this logic. Probably instead, FilePathWatcherFSEvents
should have a static bool function that does this, even on iOS. An inline in the
header is OK.

[vandebo (ex-Chrome), 2014-06-10 00:52:06]

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:27: :
mac::LibDispatchTaskRunner("chromium.org.FilePathWatcherFSEvents") {
On 2014/06/08 15:29:02, Mark Mentovai wrote:
> Do we really need another thread for this? The UI thread should be capable of
> receiving FSEvent notifications. This shouldn’t be considered dangerous
because
> there won’t be any blocking involved. The UI thread can post tasks to the IO
> thread or wherever you actually want things to be handled (as they would be on
> other platforms).

There was a thread "(OSX) Message Loops in Chrome" that you were all, the
conclusion of which was to use a dispatch queue.  I am not a Mac expert and
relied on the conclusion of that thread.

As I understand things, dispatch queues don't necessarily make a new thread, but
there is a thread pool which tasks are dispatched to - like the Chrome worker
pull.

I don't think CFRunLoopGetMain() is a complete solution for the UI thread, since
I would need to posts tasks to the UI thread. Therefore, I'd need to augment the
interface to get a reference to the UI message loop, either by changing the
Watch() method or adding a static method to set it during start up.

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:27: :
mac::LibDispatchTaskRunner("chromium.org.FilePathWatcherFSEvents") {
On 2014/06/08 15:29:02, Mark Mentovai wrote:
> These should be reverse DNS: org.chromium.whatever.

Done.

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:38: FilePath ResolvePath(const
FilePath& path) {
On 2014/06/08 15:29:02, Mark Mentovai wrote:
> Can you just call realpath()?

Hmm, it seems so.  Thanks.

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:111:
watcher->OnFilePathsChanged(paths);
On 2014/06/08 15:29:02, Mark Mentovai wrote:
> UpdateEventStream happens in the future. OnFilePathsChanged happens right
here,
> right now. If some changes to the filesystem occur in between these two
events,
> they’ll be missed.

My understanding is that the event id prevents that race.  When
UpdateEventStream() calls FSEVETNStreamCreate(), it passes |root_change_at| as
the starting point for notifications, so it doesn't matter how much wall clock
time passes.

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.h (right):

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.h:19: // implementation. The biggest
issues are that FSEvents on 10.6 sometimes drops
On 2014/06/08 15:29:02, Mark Mentovai wrote:
> FSEvents can drop events on any OS. Who are the callers and what are their
> requirements? Who can tolerate flakiness? Can anyone not tolerate flakiness?
Who
> needs file-modify events?

There are not existing callers for recursive watching because it is not
supported before this CL (all current code uses kqueue). Recursive capability
will initially be used by the media galleries API to notify consumers when files
are changed or added in a directory tree.

My understanding is that post 10.6, FSEvents is reliable. If it is not reliable,
but only drops events when they are too many in too short a time, that would
still be acceptable (we don't care exactly what changed, as long as
notifications of some change are reliable). 

Since kqueue does not trigger for file-modify events, no current consumers need
it.

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
File base/files/file_path_watcher_mac.cc (right):

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_mac.cc:27: #if !defined(OS_IOS)
On 2014/06/08 15:29:02, Mark Mentovai wrote:
> file_path_watcher_browsertest has a function RecursiveWatchAvaiable that
> basically duplicates this logic. Probably instead, FilePathWatcherFSEvents
> should have a static bool function that does this, even on iOS. An inline in
the
> header is OK.

The fsevents header needs some stuff not available on IOS, so I moved the
function into the main file_path_watcher files.

[vandebo (ex-Chrome), 2014-06-10 17:19:30]

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:38: FilePath ResolvePath(const
FilePath& path) {
On 2014/06/10 00:52:06, vandebo wrote:
> On 2014/06/08 15:29:02, Mark Mentovai wrote:
> > Can you just call realpath()?
> 
> Hmm, it seems so.  Thanks.

Actually no - I must have missed a compile or something when I tested this. 
realpath() assumes that the entire path exists.  The recursive watch tests
assume that the full path may not exist and tests that creating the final
directory triggers a watch.

[Robert Sesek, 2014-06-10 21:34:05]

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
File base/files/file_path_watcher_fsevents.cc (right):

https://codereview.chromium.org/313083007/diff/40001/base/files/file_path_wat...
base/files/file_path_watcher_fsevents.cc:27: :
mac::LibDispatchTaskRunner("chromium.org.FilePathWatcherFSEvents") {
On 2014/06/10 00:52:05, vandebo wrote:
> On 2014/06/08 15:29:02, Mark Mentovai wrote:
> > Do we really need another thread for this? The UI thread should be capable
of
> > receiving FSEvent notifications. This shouldn’t be considered dangerous
> because
> > there won’t be any blocking involved. The UI thread can post tasks to the IO
> > thread or wherever you actually want things to be handled (as they would be
on
> > other platforms).
> 
> There was a thread "(OSX) Message Loops in Chrome" that you were all, the
> conclusion of which was to use a dispatch queue.  I am not a Mac expert and
> relied on the conclusion of that thread.
> 
> As I understand things, dispatch queues don't necessarily make a new thread,
but
> there is a thread pool which tasks are dispatched to - like the Chrome worker
> pull.

This is a correct understanding. Libdispatch dynamically manages a thread pool
and guarantees execution semantics for queues, though queues can be scheduled on
arbitrary threads. We already will have Libdispatch threads running in the
browser, so this shouldn't be too bad.

> I don't think CFRunLoopGetMain() is a complete solution for the UI thread,
since
> I would need to posts tasks to the UI thread. Therefore, I'd need to augment
the
> interface to get a reference to the UI message loop, either by changing the
> Watch() method or adding a static method to set it during start up.

I think this an appropriate use of LibDispatchTaskRunner. On first glance, I
thought it would maybe be possible to just use Libdispatch/dispatch_async()
directly, even for what is currently being PostTask()ed. But because the target
object of base::Bind() is C++ ref-counted, using blocks exclusively (without
LibDispatchTaskRunner) would mess up (or make more clunky) the ref-counting of
that object. I think this is a good use of LibDispatchTaskRunner.

[Mark Mentovai, 2014-06-10 21:38:09]

LGTM. I don’t know if Jeremy planned on looking at this.

[vandebo (ex-Chrome), 2014-06-10 21:41:23]

On 2014/06/10 21:38:09, Mark Mentovai wrote:
> LGTM. I don’t know if Jeremy planned on looking at this.

Thanks.  Jeremy hasn't said anything for 5 days, so I'll proceed without him. 
Happy to address follow up comments.

[vandebo (ex-Chrome), 2014-06-10 21:44:13]

The CQ bit was checked by vandebo@chromium.org

[commit-bot: I haz the power, 2014-06-10 21:49:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/vandebo@chromium.org/313083007/100001

[jeremy, 2014-06-10 22:22:35]

Been busy, sorry - if Mark is OK with this then I am too.


On Tue, Jun 10, 2014 at 2:41 PM, <vandebo@chromium.org> wrote:

> On 2014/06/10 21:38:09, Mark Mentovai wrote:
>
>> LGTM. I don't know if Jeremy planned on looking at this.
>>
>
> Thanks.  Jeremy hasn't said anything for 5 days, so I'll proceed without
> him.
> Happy to address follow up comments.
>
> https://codereview.chromium.org/313083007/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-06-11 03:53:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-06-11 07:15:28]

Change committed as 276289