Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2332923004:
Add a new file descriptor watch API. (Closed)
DescriptionAdd a new file descriptor watch API.
This API can be used from any task from which
SequencedTaskRunnerHandle::Get() returns a SequencedTaskRunner (i.e. in
particular, it can be used from SINGLE_THREADED and SEQUENCED tasks
running in base/task_scheduler). As opposed to the existing API which
is only available from single-threaded tasks running in a
MessageLoopForIO.
Threads that want to support this API must instantiate a
FileDescriptorWatcher. The constructor of FileDescriptorWatcher takes
as argument a MessageLoopForIO to use to watch file descriptors for
which callbacks are registered on the thread on which it is invoked. The
MessageLoopForIO does *not* have to run on that thread. When the
MessageLoopForIO detects that watched file descriptors are readable
and/or writable without blocking, it posts a task to run the callback on
the sequence on which it was registered.
Design doc:
https://docs.google.com/document/d/1F5CjON2JNtCtdEug3LOL8-avj188k_xOgqzdv288Q_s/edit?usp=sharing
Discussion on chromium-dev:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/voOAab4mV9A/S9xXdpPkBgAJ
BUG=645114
Committed: https://crrev.com/f9f6c1e92b168281ba2fccef2693469380243db8
Cr-Commit-Position: refs/heads/master@{#418853}
Patch Set 1 #
Total comments: 12
Patch Set 2 : CR dcheng #8 #Patch Set 3 : self-review #
Total comments: 3
Patch Set 4 : CR dcheng # 12 #Patch Set 5 : self-review #
Messages
Total messages: 21 (10 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
fdoray@chromium.org changed reviewers: + dcheng@chromium.org
PTAL
Description was changed from ========== Add a new file descriptor watch API. This API can be used from any task from which SequencedTaskRunnerHandle::Get() returns a SequencedTaskRunner (i.e. in particular, it can be used from SINGLE_THREADED and SEQUENCED tasks running in base/task_scheduler). As opposed to the existing API which is only available from single-threaded tasks running in a MessageLoopForIO. Threads that want to support this API must instantiate a FileDescriptorWatcher. The constructor of FileDescriptorWatcher takes as argument a MessageLoopForIO to use to watch file descriptors for which callbacks are registered on the thread on which it is invoked. The MessageLoopForIO does *not* have to run on that thread. When the MessageLoopForIO detects that watched file descriptors are readable and/or writable without blocking, it posts a task to run the callback on the sequence on which it was registered. BUG=645114 ========== to ========== Add a new file descriptor watch API. This API can be used from any task from which SequencedTaskRunnerHandle::Get() returns a SequencedTaskRunner (i.e. in particular, it can be used from SINGLE_THREADED and SEQUENCED tasks running in base/task_scheduler). As opposed to the existing API which is only available from single-threaded tasks running in a MessageLoopForIO. Threads that want to support this API must instantiate a FileDescriptorWatcher. The constructor of FileDescriptorWatcher takes as argument a MessageLoopForIO to use to watch file descriptors for which callbacks are registered on the thread on which it is invoked. The MessageLoopForIO does *not* have to run on that thread. When the MessageLoopForIO detects that watched file descriptors are readable and/or writable without blocking, it posts a task to run the callback on the sequence on which it was registered. Design doc: https://docs.google.com/document/d/1F5CjON2JNtCtdEug3LOL8-avj188k_xOgqzdv288Q... Discussion on chromium-dev: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/voOAab4mV9A/S9xXd... BUG=645114 ==========
Just some initial thoughts. https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... File base/files/file_descriptor_watcher_posix.cc (right): https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:159: MessageLoopForIO::current()->AddDestructionObserver(watcher); Is it possible to just do this in the ctor? https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:170: was_deleted_ = &was_deleted; Isn't this what WeakPtr is for? WeakPtr<Controller> weak_this; And then check weak_this below? https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:173: auto callback = std::move(callback_); Why do we have to transfer ownership at all? Why can't we just leave it? https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:198: return std::unique_ptr<Controller>( WrapUnique() here and below https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... File base/files/file_descriptor_watcher_posix_unittest.cc (right): https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix_unittest.cc:73: reinterpret_cast<MessageLoopForIO*>(message_loop_for_io)); static_cast should work and look slightly less scary
PTAnL https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... File base/files/file_descriptor_watcher_posix.cc (right): https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:159: MessageLoopForIO::current()->AddDestructionObserver(watcher); On 2016/09/14 06:48:19, dcheng wrote: > Is it possible to just do this in the ctor? No. AddDestructionObserver has to be called on the MessageLoopForIO thread. Added a comment to clarify that. https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:170: was_deleted_ = &was_deleted; On 2016/09/14 06:48:19, dcheng wrote: > Isn't this what WeakPtr is for? WeakPtr<Controller> weak_this; > > And then check weak_this below? Done. You're right. https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:173: auto callback = std::move(callback_); On 2016/09/14 06:48:19, dcheng wrote: > Why do we have to transfer ownership at all? Why can't we just leave it? |callback_| can delete |this| and hence delete itself. I wasn't sure whether that was valid, but since FileDescriptorWatcherTest.DeleteControllerFromCallback succeeds, it must be. https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:198: return std::unique_ptr<Controller>( On 2016/09/14 06:48:19, dcheng wrote: > WrapUnique() here and below Done. https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... File base/files/file_descriptor_watcher_posix_unittest.cc (right): https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix_unittest.cc:73: reinterpret_cast<MessageLoopForIO*>(message_loop_for_io)); On 2016/09/14 06:48:19, dcheng wrote: > static_cast should work and look slightly less scary Done.
https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... File base/files/file_descriptor_watcher_posix.cc (right): https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:173: auto callback = std::move(callback_); On 2016/09/14 17:06:14, fdoray wrote: > On 2016/09/14 06:48:19, dcheng wrote: > > Why do we have to transfer ownership at all? Why can't we just leave it? > > |callback_| can delete |this| and hence delete itself. I wasn't sure whether > that was valid, but since FileDescriptorWatcherTest.DeleteControllerFromCallback > succeeds, it must be. Sure, but that's OK: we can just check if |this| was deleted and skip the rest of the work. We don't need to actually reference |callback_| at all after it runs, so why bother saving and restoring it?
https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... File base/files/file_descriptor_watcher_posix.cc (right): https://codereview.chromium.org/2332923004/diff/1/base/files/file_descriptor_... base/files/file_descriptor_watcher_posix.cc:173: auto callback = std::move(callback_); On 2016/09/14 17:13:17, dcheng wrote: > On 2016/09/14 17:06:14, fdoray wrote: > > On 2016/09/14 06:48:19, dcheng wrote: > > > Why do we have to transfer ownership at all? Why can't we just leave it? > > > > |callback_| can delete |this| and hence delete itself. I wasn't sure whether > > that was valid, but since > FileDescriptorWatcherTest.DeleteControllerFromCallback > > succeeds, it must be. > > Sure, but that's OK: we can just check if |this| was deleted and skip the rest > of the work. We don't need to actually reference |callback_| at all after it > runs, so why bother saving and restoring it? In the latest patch set, I no longer save/restore callback_.
LGTM with comment addressed. https://codereview.chromium.org/2332923004/diff/40001/base/files/file_descrip... File base/files/file_descriptor_watcher_posix.cc (right): https://codereview.chromium.org/2332923004/diff/40001/base/files/file_descrip... base/files/file_descriptor_watcher_posix.cc:159: Unretained(watcher_.get()), called_from_constructor)); Nit: let's just make this an actual method on Watcher (I think this lets us get rid of the friend declaration too?) This also isn't really a pattern I care to encourage, since it actual disables automatic enforcement of the receiver pointer: I believe that as written, this will compile fine without the Unretained() annotation.
https://codereview.chromium.org/2332923004/diff/40001/base/files/file_descrip... File base/files/file_descriptor_watcher_posix.cc (right): https://codereview.chromium.org/2332923004/diff/40001/base/files/file_descrip... base/files/file_descriptor_watcher_posix.cc:159: Unretained(watcher_.get()), called_from_constructor)); On 2016/09/15 08:23:02, dcheng wrote: > Nit: let's just make this an actual method on Watcher (I think this lets us get > rid of the friend declaration too?) > > This also isn't really a pattern I care to encourage, since it actual disables > automatic enforcement of the receiver pointer: I believe that as written, this > will compile fine without the Unretained() annotation. Done.
https://codereview.chromium.org/2332923004/diff/40001/base/files/file_descrip... File base/files/file_descriptor_watcher_posix.cc (right): https://codereview.chromium.org/2332923004/diff/40001/base/files/file_descrip... base/files/file_descriptor_watcher_posix.cc:159: Unretained(watcher_.get()), called_from_constructor)); On 2016/09/15 08:23:02, dcheng wrote: > Nit: let's just make this an actual method on Watcher (I think this lets us get > rid of the friend declaration too?) > > This also isn't really a pattern I care to encourage, since it actual disables > automatic enforcement of the receiver pointer: I believe that as written, this > will compile fine without the Unretained() annotation. Done. Note that Unretained() is still required because watcher_ isn't ref-counted or weakptr. I added a comment about why it is safe to use Unretained().
The CQ bit was checked by fdoray@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2332923004/#ps80001 (title: "self-review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a new file descriptor watch API. This API can be used from any task from which SequencedTaskRunnerHandle::Get() returns a SequencedTaskRunner (i.e. in particular, it can be used from SINGLE_THREADED and SEQUENCED tasks running in base/task_scheduler). As opposed to the existing API which is only available from single-threaded tasks running in a MessageLoopForIO. Threads that want to support this API must instantiate a FileDescriptorWatcher. The constructor of FileDescriptorWatcher takes as argument a MessageLoopForIO to use to watch file descriptors for which callbacks are registered on the thread on which it is invoked. The MessageLoopForIO does *not* have to run on that thread. When the MessageLoopForIO detects that watched file descriptors are readable and/or writable without blocking, it posts a task to run the callback on the sequence on which it was registered. Design doc: https://docs.google.com/document/d/1F5CjON2JNtCtdEug3LOL8-avj188k_xOgqzdv288Q... Discussion on chromium-dev: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/voOAab4mV9A/S9xXd... BUG=645114 ========== to ========== Add a new file descriptor watch API. This API can be used from any task from which SequencedTaskRunnerHandle::Get() returns a SequencedTaskRunner (i.e. in particular, it can be used from SINGLE_THREADED and SEQUENCED tasks running in base/task_scheduler). As opposed to the existing API which is only available from single-threaded tasks running in a MessageLoopForIO. Threads that want to support this API must instantiate a FileDescriptorWatcher. The constructor of FileDescriptorWatcher takes as argument a MessageLoopForIO to use to watch file descriptors for which callbacks are registered on the thread on which it is invoked. The MessageLoopForIO does *not* have to run on that thread. When the MessageLoopForIO detects that watched file descriptors are readable and/or writable without blocking, it posts a task to run the callback on the sequence on which it was registered. Design doc: https://docs.google.com/document/d/1F5CjON2JNtCtdEug3LOL8-avj188k_xOgqzdv288Q... Discussion on chromium-dev: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/voOAab4mV9A/S9xXd... BUG=645114 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add a new file descriptor watch API. This API can be used from any task from which SequencedTaskRunnerHandle::Get() returns a SequencedTaskRunner (i.e. in particular, it can be used from SINGLE_THREADED and SEQUENCED tasks running in base/task_scheduler). As opposed to the existing API which is only available from single-threaded tasks running in a MessageLoopForIO. Threads that want to support this API must instantiate a FileDescriptorWatcher. The constructor of FileDescriptorWatcher takes as argument a MessageLoopForIO to use to watch file descriptors for which callbacks are registered on the thread on which it is invoked. The MessageLoopForIO does *not* have to run on that thread. When the MessageLoopForIO detects that watched file descriptors are readable and/or writable without blocking, it posts a task to run the callback on the sequence on which it was registered. Design doc: https://docs.google.com/document/d/1F5CjON2JNtCtdEug3LOL8-avj188k_xOgqzdv288Q... Discussion on chromium-dev: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/voOAab4mV9A/S9xXd... BUG=645114 ========== to ========== Add a new file descriptor watch API. This API can be used from any task from which SequencedTaskRunnerHandle::Get() returns a SequencedTaskRunner (i.e. in particular, it can be used from SINGLE_THREADED and SEQUENCED tasks running in base/task_scheduler). As opposed to the existing API which is only available from single-threaded tasks running in a MessageLoopForIO. Threads that want to support this API must instantiate a FileDescriptorWatcher. The constructor of FileDescriptorWatcher takes as argument a MessageLoopForIO to use to watch file descriptors for which callbacks are registered on the thread on which it is invoked. The MessageLoopForIO does *not* have to run on that thread. When the MessageLoopForIO detects that watched file descriptors are readable and/or writable without blocking, it posts a task to run the callback on the sequence on which it was registered. Design doc: https://docs.google.com/document/d/1F5CjON2JNtCtdEug3LOL8-avj188k_xOgqzdv288Q... Discussion on chromium-dev: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/voOAab4mV9A/S9xXd... BUG=645114 Committed: https://crrev.com/f9f6c1e92b168281ba2fccef2693469380243db8 Cr-Commit-Position: refs/heads/master@{#418853} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f9f6c1e92b168281ba2fccef2693469380243db8 Cr-Commit-Position: refs/heads/master@{#418853} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
