Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1046353004:
Access fields from the appropriate threads in FilePathWatcherFSEvents. (Closed)
|
Created:
5 years, 8 months ago by Reilly Grant (use Gerrit) Modified:
5 years, 8 months ago Reviewers:
Mattias Nissler (ping if slow) CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAccess fields from the appropriate threads in FilePathWatcherFSEvents.
Limit reads and writes to the target_, resolved_target_ and callback_
fields to the libdispatch task runner only. This should resolve
possible races between calls to FilePathWatcherFSEvents::Watch and
incoming events.
BUG=457793
Committed: https://crrev.com/6422ac627ba05c4cbcbdec5a80484e5b8859028a
Cr-Commit-Position: refs/heads/master@{#324706}
Patch Set 1 : #
Total comments: 7
Patch Set 2 : Use callback_ and resolved_target_ as sentinels. #
Total comments: 6
Patch Set 3 : Protect access to callback_ and add comments about thread usage. #Patch Set 4 : Check callback_ instead of fsevent_stream_ in destructor. #
Total comments: 8
Patch Set 5 : Add comments about thread usage. #
Messages
Total messages: 17 (3 generated)
Patchset #1 (id:1) has been deleted
reillyg@chromium.org changed reviewers: + mnissler@chromium.org
Please take a look. This should resolve the issues you pointed out. Thanks for the help!
https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.cc (left): https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:200: // It can happen that the watcher gets canceled while tasks that call this I don't see why we no longer need a safeguard here, can you explain? I think FSEventsCallback can still post a task to this function and in the mean time we might execute a cancel task. And then, we'd accidentally recreate the event stream when the UpdateEventStream task runs. You might be able to use |target_| as a sentinel. https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.cc (right): https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:120: callback.Run(target, false); I think this may result in callbacks firing even after FilePathWatcherFSEvents::Cancel() was invoked, which is against the contract. Or am I missing something? Btw., do we have a test case for this? https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:241: callback_ = callback; I think you want to keep callback_ on the message_loop() thread, so you can nuke it on Cancel(), so you can reliably stop running callbacks after Cancel() gets called on the message_loop() thread. https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.h (right): https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.h:58: // Callback to notify upon changes. Please also document here which thread the members should be accessed from so people touching this file in the future will actually have a chance to obey the rules :)
https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.cc (left): https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:200: // It can happen that the watcher gets canceled while tasks that call this On 2015/04/02 17:35:45, Mattias Nissler wrote: > I don't see why we no longer need a safeguard here, can you explain? I think > FSEventsCallback can still post a task to this function and in the mean time we > might execute a cancel task. And then, we'd accidentally recreate the event > stream when the UpdateEventStream task runs. You might be able to use |target_| > as a sentinel. With the cancel operation serialized onto the libdispatch thread it will either run before FSEventsCallback is delivered, preventing it from being called or after. The issue is the callbacks after cancel mentioned above. https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.cc (right): https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:120: callback.Run(target, false); On 2015/04/02 17:35:45, Mattias Nissler wrote: > I think this may result in callbacks firing even after > FilePathWatcherFSEvents::Cancel() was invoked, which is against the contract. Or > am I missing something? Btw., do we have a test case for this? That's true. If it's a contract violation then I'll come up with a way to safely invalidate the callback across threads. https://codereview.chromium.org/1046353004/diff/20001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:241: callback_ = callback; On 2015/04/02 17:35:45, Mattias Nissler wrote: > I think you want to keep callback_ on the message_loop() thread, so you can nuke > it on Cancel(), so you can reliably stop running callbacks after Cancel() gets > called on the message_loop() thread. I don't think base::Callback is thread safe. I'll find something that is to use as a sentinel.
Thank you for the feedback. I've updated the code to use callback_ as a sentinel on the message_loop() thread and resolved_target_ as a sentinel on the g_task_runner thread. I think this should resolve your concerns.
This is definitely moving in the right direction now. https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.cc (right): https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:155: DCHECK(!fsevent_stream_) Is the dtor guaranteed to run on the dispatch queue thread? Otherwise the reference to fsevent_stream_ here may be from the wrong thread. https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:236: message_loop()->PostTask(FROM_HERE, Bind(callback_, target_, true)); Strictly speaking, this is still referencing callback_ from the wrong thread (same in line 245 below). Probably makes sense to bump this through a helper on the message_loop() thread which can safely fill in callback_. Might just call it ReportError() for clarity. https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.h (right): https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.h:69: Would still be good to add comments here to indicate thread access rules for individual data members.
https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.cc (right): https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:155: DCHECK(!fsevent_stream_) On 2015/04/07 11:36:20, Mattias Nissler wrote: > Is the dtor guaranteed to run on the dispatch queue thread? Otherwise the > reference to fsevent_stream_ here may be from the wrong thread. There are a couple of cases: 1) If Cancel has not been called then the last reference is released on the message_loop thread then this will be called here and the assertion will trigger. We can guarantee that Cancel won't be called after the last reference is released so checking fsevent_stream_ is reasonably safe and we're about to hit a DCHECK anyways. 2) If Cancel has been called then there are at least two references. One on the message_loop thread and one held by the callback posted by Cancel to the libdispatch thread. If the former is released first then this path isn't hit. If the later is released first then when DestroyEventStream completes the destructor is called on the libdispatch thread and fsevent_stream_ is guaranteed to be nullptr. https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:236: message_loop()->PostTask(FROM_HERE, Bind(callback_, target_, true)); On 2015/04/07 11:36:20, Mattias Nissler wrote: > Strictly speaking, this is still referencing callback_ from the wrong thread > (same in line 245 below). Probably makes sense to bump this through a helper on > the message_loop() thread which can safely fill in callback_. Might just call it > ReportError() for clarity. Done.
https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.cc (right): https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:155: DCHECK(!fsevent_stream_) On 2015/04/07 20:42:14, reillyg wrote: > On 2015/04/07 11:36:20, Mattias Nissler wrote: > > Is the dtor guaranteed to run on the dispatch queue thread? Otherwise the > > reference to fsevent_stream_ here may be from the wrong thread. > > There are a couple of cases: > > 1) If Cancel has not been called then the last reference is released on the > message_loop thread then this will be called here and the assertion will > trigger. We can guarantee that Cancel won't be called after the last reference > is released so checking fsevent_stream_ is reasonably safe and we're about to > hit a DCHECK anyways. > 2) If Cancel has been called then there are at least two references. One on the > message_loop thread and one held by the callback posted by Cancel to the > libdispatch thread. If the former is released first then this path isn't hit. If > the later is released first then when DestroyEventStream completes the > destructor is called on the libdispatch thread and fsevent_stream_ is guaranteed > to be nullptr. I don't see why you would be guaranteed that fsevent_stream_ is nullptr on the message_loop() thread. Say the reference on the libdispatch thread gets dropped first and |fsevent_stream_| reset to nullptr there. If the reference on the message_loop() thread gets dropped shortly after, the |fsevent_stream_| pointer as observed on the message_loop() thread may still not have picked up the nullptr value (unless there's a synchronization point I'm missing).
On 2015/04/08 07:57:10, Mattias Nissler wrote: > https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... > File base/files/file_path_watcher_fsevents.cc (right): > > https://codereview.chromium.org/1046353004/diff/40001/base/files/file_path_wa... > base/files/file_path_watcher_fsevents.cc:155: DCHECK(!fsevent_stream_) > On 2015/04/07 20:42:14, reillyg wrote: > > On 2015/04/07 11:36:20, Mattias Nissler wrote: > > > Is the dtor guaranteed to run on the dispatch queue thread? Otherwise the > > > reference to fsevent_stream_ here may be from the wrong thread. > > > > There are a couple of cases: > > > > 1) If Cancel has not been called then the last reference is released on the > > message_loop thread then this will be called here and the assertion will > > trigger. We can guarantee that Cancel won't be called after the last reference > > is released so checking fsevent_stream_ is reasonably safe and we're about to > > hit a DCHECK anyways. > > 2) If Cancel has been called then there are at least two references. One on > the > > message_loop thread and one held by the callback posted by Cancel to the > > libdispatch thread. If the former is released first then this path isn't hit. > If > > the later is released first then when DestroyEventStream completes the > > destructor is called on the libdispatch thread and fsevent_stream_ is > guaranteed > > to be nullptr. > > I don't see why you would be guaranteed that fsevent_stream_ is nullptr on the > message_loop() thread. Say the reference on the libdispatch thread gets dropped > first and |fsevent_stream_| reset to nullptr there. If the reference on the > message_loop() thread gets dropped shortly after, the |fsevent_stream_| pointer > as observed on the message_loop() thread may still not have picked up the > nullptr value (unless there's a synchronization point I'm missing). Good point (though if I pulled out a copy of the Intel manuals I think I could show that the atomic operations on the reference count provide enough of a memory barrier). I'll switch to checking callback_ as that should provide roughly the same signal.
https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.cc (right): https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:155: DCHECK(callback_.is_null()) Now, if the object gets destroyed on the g_task_runner thread, you can't be sure that callback_ is up-to-date on that thread. Unless the reference counting indeed provides a memory barrier on all relevant platforms... Please at least add a comment here to explain why the member access from the wrong thread is OK here. An alternative thing you could do is checking either |callback_| or |fsevent_stream_| depending on the thread you're on - but that's not pretty either. https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:160: const std::vector<FilePath>& paths) { nit: DCHECK(g_task_runner.Get().RunsTasksOnCurrentThread()); https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:253: void FilePathWatcherFSEvents::ReportError(const FilePath& target) { nit: DCHECK(message_loop()->RunsTasksOnCurrentThread()); https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.h (right): https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.h:73: // Callback to notify upon changes. 3rd attempt: Please document in the comments which thread these members are accessed on.
https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.cc (right): https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:155: DCHECK(callback_.is_null()) On 2015/04/10 09:15:51, Mattias Nissler wrote: > Now, if the object gets destroyed on the g_task_runner thread, you can't be sure > that callback_ is up-to-date on that thread. Unless the reference counting > indeed provides a memory barrier on all relevant platforms... Please at least > add a comment here to explain why the member access from the wrong thread is OK > here. > > An alternative thing you could do is checking either |callback_| or > |fsevent_stream_| depending on the thread you're on - but that's not pretty > either. Reference counting doesn't provide a sufficient memory barrier but the PostTask in Cancel() does. I'll add a comment to this effect. https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:160: const std::vector<FilePath>& paths) { On 2015/04/10 09:15:51, Mattias Nissler wrote: > nit: DCHECK(g_task_runner.Get().RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.cc:253: void FilePathWatcherFSEvents::ReportError(const FilePath& target) { On 2015/04/10 09:15:51, Mattias Nissler wrote: > nit: DCHECK(message_loop()->RunsTasksOnCurrentThread()); Done. https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... File base/files/file_path_watcher_fsevents.h (right): https://codereview.chromium.org/1046353004/diff/80001/base/files/file_path_wa... base/files/file_path_watcher_fsevents.h:73: // Callback to notify upon changes. On 2015/04/10 09:15:51, Mattias Nissler wrote: > 3rd attempt: Please document in the comments which thread these members are > accessed on. My mistake. I put in comments but must not have saved the header (since there were no other changes) before uploading the review.
LGTM, thanks!
The CQ bit was checked by reillyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1046353004/100001
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6422ac627ba05c4cbcbdec5a80484e5b8859028a Cr-Commit-Position: refs/heads/master@{#324706} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
