Issue metadata
Sign in to add a comment
|
Mojo C++: remove message loop destruction observer from mojo::Watcher |
||||||||||||||||||||||||
Issue descriptionCurrently, when the message loop is destructed, a watching mojo::Watcher will notify handle ready with MOJO_RESULT_ABORTED. I would like to remove this behavior, because: - This is not part of the underlying system API behavior. - It slows down message loop shutdown. - The notification doesn't guarantee to trigger the connection error handler of the corresponding InterfacePtr/Binding. (E.g., there are pending messages to be processed so the connection error is delayed which would never happen.) If users need to do some cleanup on message loop shutdown, they need to add observer in their code instead. - If there are multiple task runners on the same thread, we don't know from which task runner the message loop destruction observer is run, so when we support binding InterfacePtr/Binding to a specific task runner, we have no way to make sure we notify handle ready on the correct task runner.
,
Apr 20 2016
So what happens to a pending mojo method call when the message loop shuts down? Now, neither the callback, nor connection error handler run and the call is left hanging. Having every user of Mojo have to register their own message loop destruction observer in order to clean up correctly seems problematic.
,
Apr 20 2016
Hmm good point. I couldn't remember why we had to make Watcher a MessageLoop DestructionObserver, but that's why. Anyone relying on connection errors for cleanup (e.g. all users of StrongBinding) would otherwise leak if still around during shutdown.
,
Apr 20 2016
The current code doesn't guarantee that this mojo::Watcher notification on MessageLoop destruction always trigger OnConnectionError. It is just rare enough that we never get into that situation with our test coverage. The current behavior is: if there are incoming messages queued, the connection error notification will be delayed until we finish dispatching those queued messages to the user. (That is similar to our message pipe semantics -- you won't observe a pipe error until no data to read.) At message loop destruction time, it is possible that we won't be able to finish processing all queued messages to signal connection error. Another scenario: An associated endpoint EndpointA lives on thread ThreadA, the corresponding master endpoint EndpointM lives on thread ThreadM. If ThreadA goes away before ThreadM, because EndpointA relies on EndpointM to receive messages, it doesn't have a Watcher at all, so it won't trigger a connection error. Besides, I feel that message loop destruction is something different from pipe disconnection. On message loop destruction, the user could extract the underlying message pipe and pass it to another thread to rebind there. :) That is why I feel that the observation of message loop destruction should live at a higher level. It may be as low as interface endpoints (that is, [associated] InterfacePtr/Binding), but not at the current level. (And even if it is at the interface endpoint level, I still don't know how to make sure notifying connection error on the right task runner if there are multiple on the same thread. :/ Maybe we should just allow error notification on any task runner of that thread?) I feel that it may make more sense to live at the application layer, which has the responsibility to manage lifespan of InterfacePtr/Binding it owns. StrongBinding is a utility that helps the application to manage lifespan of a Binding, so we can let itself be a MessageLoop destruction observer. WDYT? Thanks!
,
Apr 20 2016
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3f9ef16903199e305ae2ffb3fe74fcdd48e2ded0 commit 3f9ef16903199e305ae2ffb3fe74fcdd48e2ded0 Author: yzshen <yzshen@chromium.org> Date: Tue Apr 26 16:35:18 2016 Mojo C++ bindings: make StrongBinding a message loop destruction observer. This is a preparation for removing message loop destruction observer from mojo::Watcher. BUG= 604762 Review URL: https://codereview.chromium.org/1913333002 Cr-Commit-Position: refs/heads/master@{#389803} [modify] https://crrev.com/3f9ef16903199e305ae2ffb3fe74fcdd48e2ded0/chrome/utility/image_decoder_impl_unittest.cc [modify] https://crrev.com/3f9ef16903199e305ae2ffb3fe74fcdd48e2ded0/mojo/public/cpp/bindings/strong_binding.h
,
Apr 26 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/0950cefe4e9c89fd11c4077123b9084c8173fdd9 commit 0950cefe4e9c89fd11c4077123b9084c8173fdd9 Author: yzshen <yzshen@chromium.org> Date: Tue Apr 26 22:39:33 2016 Mojo: Make device/ to not rely on connecton error notification on message loop destruction. - Make device/usb to use StrongBinding wherever possible. - Remove unnecessary DCHECK from DataSourceSender. This is a preparation for removing connection error notification on message loop destruction. BUG= 604762 Review URL: https://codereview.chromium.org/1925463002 Cr-Commit-Position: refs/heads/master@{#389926} [modify] https://crrev.com/0950cefe4e9c89fd11c4077123b9084c8173fdd9/device/serial/data_source_sender.cc [modify] https://crrev.com/0950cefe4e9c89fd11c4077123b9084c8173fdd9/device/usb/mojo/device_impl.cc [modify] https://crrev.com/0950cefe4e9c89fd11c4077123b9084c8173fdd9/device/usb/mojo/device_impl.h [modify] https://crrev.com/0950cefe4e9c89fd11c4077123b9084c8173fdd9/device/usb/mojo/device_manager_impl.cc [modify] https://crrev.com/0950cefe4e9c89fd11c4077123b9084c8173fdd9/device/usb/mojo/device_manager_impl.h
,
May 19 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ba42c515d544ebd6922b1f6064da20f6ac7ecbd4 commit ba42c515d544ebd6922b1f6064da20f6ac7ecbd4 Author: yzshen <yzshen@chromium.org> Date: Thu May 19 02:05:22 2016 Revert of Mojo C++ bindings: make StrongBinding a message loop destruction observer. (patchset #2 id:20001 of https://codereview.chromium.org/1913333002/ ) Reason for revert: As discussed in the mailing thread, I am thinking to do destruction observation at the router level instead. Original issue's description: > Mojo C++ bindings: make StrongBinding a message loop destruction observer. > > This is a preparation for removing message loop destruction observer from mojo::Watcher. > > BUG= 604762 > > Committed: https://crrev.com/3f9ef16903199e305ae2ffb3fe74fcdd48e2ded0 > Cr-Commit-Position: refs/heads/master@{#389803} TBR=rockot@chromium.org,jam@chromium.org # Not skipping CQ checks because original CL landed more than 1 days ago. BUG= 604762 Review-Url: https://codereview.chromium.org/1993063003 Cr-Commit-Position: refs/heads/master@{#394635} [modify] https://crrev.com/ba42c515d544ebd6922b1f6064da20f6ac7ecbd4/chrome/utility/image_decoder_impl_unittest.cc [modify] https://crrev.com/ba42c515d544ebd6922b1f6064da20f6ac7ecbd4/mojo/public/cpp/bindings/strong_binding.h
,
Sep 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/421e2d6ed8e31318f8e8e653d2d892d9b58815df commit 421e2d6ed8e31318f8e8e653d2d892d9b58815df Author: yzshen <yzshen@chromium.org> Date: Wed Sep 21 15:37:59 2016 Mojo C++ bindings: make InterfaceEndpointClient observe message loop destruction. Otherwise, there is no guarantee that we will issue connection error notification on message loop destruction. The observer of mojo::Watcher will need to be removed. BUG= 594244 , 604762 Review-Url: https://codereview.chromium.org/2358693002 Cr-Commit-Position: refs/heads/master@{#420069} [modify] https://crrev.com/421e2d6ed8e31318f8e8e653d2d892d9b58815df/mojo/public/cpp/bindings/interface_endpoint_client.h [modify] https://crrev.com/421e2d6ed8e31318f8e8e653d2d892d9b58815df/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
,
Oct 6 2016
The FILE thread will soon be migrated to TaskScheduler. TaskScheduler doesn't support destruction observers since its threads are never joined. Can you confirm that the call to AddDestrutionObserver() in mojo::Watcher::MessageLoopObserver will soon be removed? mojo::Watcher::MessageLoopObserver is used on the file thread: #2 0x000001fc6035 base::MessageLoop::AddDestructionObserver() #3 0x000002180f56 mojo::Watcher::Start() #4 0x000001d5dc3e mojo::Connector::WaitToReadMore() #5 0x000001d5dab0 mojo::Connector::Connector() #6 0x000001d643e0 mojo::internal::MultiplexRouter::MultiplexRouter() #7 0x000001d5d78e mojo::internal::MultiplexedBindingState::BindInternal() #8 0x0000019b231c mojo::internal::BindingState<>::Bind() #9 0x0000019b2201 mojo::Binding<>::Binding() #10 0x0000019b20ef mojo::StrongBinding<>::StrongBinding() #11 0x0000019b2024 mojo::StrongBinding<>::Create() #12 0x0000019b1ce9 content::MimeRegistryImpl::Create() #13 0x0000004c2084 _ZN4base8internal7InvokerINS0_9BindStateIPFvN4mojo16InterfaceRequestIN6device14BatteryMonitorEEEEJEEES8_E3RunEPNS0_13BindStateBaseEOS7_ #14 0x0000004c1b03 shell::internal::CallbackBinder<>::RunCallbackOnTaskRunner() #15 0x0000004c1f00 _ZN4base8internal7InvokerINS0_9BindStateIPFvRKNS_8CallbackIFvN4mojo16InterfaceRequestIN6device14BatteryMonitorEEEELNS0_8CopyModeE1ELNS0_10RepeatModeE1EEES8_EJSC_NS0_13PassedWrapperIS8_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #16 0x0000020313f4 base::debug::TaskAnnotator::RunTask() #17 0x000001fc754b base::MessageLoop::RunTask() #18 0x000001fc7858 base::MessageLoop::DeferOrRunPendingTask() #19 0x000001fc7bab base::MessageLoop::DoWork() #20 0x000001fc9fc9 base::MessagePumpLibevent::Run() #21 0x000001fc722e base::MessageLoop::RunHandler() #22 0x000001fe4480 base::RunLoop::Run() #23 0x0000020034cf base::Thread::Run() #24 0x0000017ba2d6 content::BrowserThreadImpl::FileThreadRun() #25 0x0000017ba6d2 content::BrowserThreadImpl::Run() #26 0x0000020039a4 base::Thread::ThreadMain()
,
Oct 6 2016
Cannot confirm that this will soon be removed. We don't yet have a good answer for what to do instead. We need to be able to notify bindings users when their endpoints are no longer functioning, which means we need to know when an endpoint can no longer schedule dispatch tasks.
,
Nov 7 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/be675544ee93b8f940230861a8a4ae1dbbf2ebc7 commit be675544ee93b8f940230861a8a4ae1dbbf2ebc7 Author: shimazu <shimazu@chromium.org> Date: Mon Nov 07 01:38:13 2016 ServiceWorker: Add a check if error handler runs on the right thread As observation in https://crbug.com/658702 , I found the mojo error handler (EWInstance::Detach in this case) is called on the UI thread when the message loop on the UI thread gets destructed though EWInstance is on the IO thread. This patch is to check the running thread before the error handling to avoid unexpected threading issues. BUG= 658702 , 604762 Review-Url: https://codereview.chromium.org/2467223002 Cr-Commit-Position: refs/heads/master@{#430208} [modify] https://crrev.com/be675544ee93b8f940230861a8a4ae1dbbf2ebc7/content/browser/service_worker/embedded_worker_instance.cc
,
Nov 7 2016
Just like rockot@ said, I was thinking to move the destruction observer to another class (InterfaceEndpointClient) from mojo::Watcher. That won't eliminate usage of destruction observer. Although the FILE thread runs till the end of the process, destruction notification may still be necessary for users to cleanup in some cases?
,
Nov 7 2016
,
Nov 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/da6bd89a95fc8c444720f3d76ac67b91dfd70280 commit da6bd89a95fc8c444720f3d76ac67b91dfd70280 Author: fdoray <fdoray@chromium.org> Date: Tue Nov 08 16:57:16 2016 Remove MessageLoop destruction observer from mojo::Watcher. mojo::Watcher currently registers itself as a destruction observer of the MessageLoop on which it is instantiated. When it is notified that the MessageLoop is being destroyed, it invokes its callback with MOJO_RESULT_ABORTED. This CL removes the MessageLoop destruction observer from mojo::Watcher. This allows mojo::Watcher to be instantiated on a thread that doesn't run a MessageLoop (e.g. TaskScheduler threads). No existing code depends on the MOJO_RESULT_ABORTED notification. BUG= 604762 Review-Url: https://codereview.chromium.org/2475273003 Cr-Commit-Position: refs/heads/master@{#430637} [modify] https://crrev.com/da6bd89a95fc8c444720f3d76ac67b91dfd70280/ipc/ipc_sync_channel.cc [modify] https://crrev.com/da6bd89a95fc8c444720f3d76ac67b91dfd70280/mojo/public/cpp/system/tests/watcher_unittest.cc [modify] https://crrev.com/da6bd89a95fc8c444720f3d76ac67b91dfd70280/mojo/public/cpp/system/watcher.cc [modify] https://crrev.com/da6bd89a95fc8c444720f3d76ac67b91dfd70280/mojo/public/cpp/system/watcher.h
,
Nov 8 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ee88cfeecda2a6fbc58ac7d2635e048ed39f28cf commit ee88cfeecda2a6fbc58ac7d2635e048ed39f28cf Author: xlai <xlai@chromium.org> Date: Tue Nov 08 19:05:11 2016 Revert of Remove MessageLoop destruction observer from mojo::Watcher. (patchset #2 id:20001 of https://codereview.chromium.org/2475273003/ ) Reason for revert: I'm suspecting that this CL is causing these two layout tests to crash on WebKit Linux Precise Leak bot: broadcastchannel/blobs.html broadcastchannel/workers.html because they are crashing on mojo::SyncHandleRegistry in a check for watcher pointer. https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Precise%20Leak/builds/1578 And this CL is the only CL in the build that touches on mojo watcher. Original issue's description: > Remove MessageLoop destruction observer from mojo::Watcher. > > mojo::Watcher currently registers itself as a destruction observer > of the MessageLoop on which it is instantiated. When it is notified > that the MessageLoop is being destroyed, it invokes its callback > with MOJO_RESULT_ABORTED. > > This CL removes the MessageLoop destruction observer from > mojo::Watcher. This allows mojo::Watcher to be instantiated on a > thread that doesn't run a MessageLoop (e.g. TaskScheduler threads). > No existing code depends on the MOJO_RESULT_ABORTED notification. > > BUG= 604762 > > Committed: https://crrev.com/da6bd89a95fc8c444720f3d76ac67b91dfd70280 > Cr-Commit-Position: refs/heads/master@{#430637} TBR=rockot@chromium.org,yzshen@chromium.org,fdoray@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 604762 Review-Url: https://codereview.chromium.org/2480153004 Cr-Commit-Position: refs/heads/master@{#430675} [modify] https://crrev.com/ee88cfeecda2a6fbc58ac7d2635e048ed39f28cf/ipc/ipc_sync_channel.cc [modify] https://crrev.com/ee88cfeecda2a6fbc58ac7d2635e048ed39f28cf/mojo/public/cpp/system/tests/watcher_unittest.cc [modify] https://crrev.com/ee88cfeecda2a6fbc58ac7d2635e048ed39f28cf/mojo/public/cpp/system/watcher.cc [modify] https://crrev.com/ee88cfeecda2a6fbc58ac7d2635e048ed39f28cf/mojo/public/cpp/system/watcher.h
,
Nov 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cfb34bcc95a51e4052f2abbef8969f46d7980464 commit cfb34bcc95a51e4052f2abbef8969f46d7980464 Author: yzshen <yzshen@chromium.org> Date: Wed Nov 30 19:26:49 2016 Mojo C++ bindings: fix [D]CHECKs in MultiplexRouter and SyncHandleRegistry. BUG= 604762 TEST=1) Remove message loop destruction observer from mojo::Watcher. 2) Run layout tests using python third_party/WebKit/Tools/Scripts/run-webkit-tests broadcastchannel/blobs.html --enable-leak-detection 3) The test should consistently pass. Review-Url: https://codereview.chromium.org/2536323002 Cr-Commit-Position: refs/heads/master@{#435374} [modify] https://crrev.com/cfb34bcc95a51e4052f2abbef8969f46d7980464/mojo/public/cpp/bindings/lib/multiplex_router.cc [modify] https://crrev.com/cfb34bcc95a51e4052f2abbef8969f46d7980464/mojo/public/cpp/bindings/lib/sync_handle_registry.cc
,
Nov 30 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1bd2370149a46bae21fecf9e98842ff51e8f5299 commit 1bd2370149a46bae21fecf9e98842ff51e8f5299 Author: yzshen <yzshen@chromium.org> Date: Wed Nov 30 21:54:51 2016 Remove MessageLoop destruction observer from mojo::Watcher. This CL is based on fdoray's CL http://crrev.com/2475273003 The only difference is this CL adds a message loop destruction observer for the Java watcher implementation, in order to preserve its original behavior. BUG= 604762 Review-Url: https://codereview.chromium.org/2540903002 Cr-Commit-Position: refs/heads/master@{#435432} [modify] https://crrev.com/1bd2370149a46bae21fecf9e98842ff51e8f5299/ipc/ipc_sync_channel.cc [modify] https://crrev.com/1bd2370149a46bae21fecf9e98842ff51e8f5299/mojo/android/system/watcher_impl.cc [modify] https://crrev.com/1bd2370149a46bae21fecf9e98842ff51e8f5299/mojo/public/cpp/system/tests/watcher_unittest.cc [modify] https://crrev.com/1bd2370149a46bae21fecf9e98842ff51e8f5299/mojo/public/cpp/system/watcher.cc [modify] https://crrev.com/1bd2370149a46bae21fecf9e98842ff51e8f5299/mojo/public/cpp/system/watcher.h
,
Dec 1 2016
,
Nov 7 2017
,
Nov 7 2017
Apologies, applied the wrong component in bulk. |
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by bugdroid1@chromium.org
, Apr 19 2016