WrapCallbackWithDefaultInvokeIfNotRun

[Łukasz Anforowicz]

Hello chromium-mojo@,

(Cc-ing petewil@ because the context of this question is the current QR Code Generator project;  Cc-ing gab@ because this question is somewhat related to TaskRunners)

Doc comments of WrapCallbackWithDefaultInvokeIfNotRun say that "the default form of the callback [...] may not run on the thread you expected" (i.e. the callback may be destructed on a different thread than the thread where Run is typically called).  In particular, I assume that mojo async response would typically be run on the same thread that the mojo::Remote is bound to (e.g. the UI thread), while the callback destruction may happen on the IO thread (where I assume mojo connectivity is managed).  Is there a recommended pattern or additional helpers to guarantee that the default form will be invoked on a specific thread (or sequence, or TaskRunner, or whatever the current technology is called :-))?

Initially I thought that maybe I can wrap `orignal_callback` (the first argument of WrapCallbackWithDefaultInvokeIfNotRun) with something like: base::BindOnce(PostToOriginalThread, std::move(original_callback), original_task_runner).  But then I've looked at base/threading/post_task_and_reply_impl.cc and it seems that calculating the `original_task_runner` may end up quite complicated.  Or... maybe it is okay to just get one through SequencedTaskRunner::GetCurrentDefault?

FWIW, the current callers of WrapCallbackWithDefaultInvokeIfNotRun (the few that I briefly looked at) seem to not care about the threading caveat (at least there are no comments that explain why the threading caveat doesn't apply + there are no threading-related wrappers).  Are these potential bugs?

Thanks,

Lukasz

[Xiaohan Wang (王消寒)]

I assumed that when mojo disconnection happens, the mojo async callback will be dropped on the thread/sequence where the `Remote` is bound to. +Ken Rockot to correct me if I am wrong. With that, the current `WrapCallbackWithDefaultInvokeIfNotRun` should suffice most use cases.

In the case where a callback may be dropped on a different thread, you can wrap the returned callback further with `base::BindPostTask`, where the input `callback` will always attempt to be destroyed on the target task runner:

https://source.chromium.org/chromium/chromium/src/+/main:base/task/bind_post_task.h;l=51-52

Best,

Xiaohan

--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/CAA_NCUEX9ULK3vnNg9MWgFuXZR6u%3D_ar1x1z_RbAbOfYZXV7Nw%40mail.gmail.com.

[Łukasz Anforowicz]

On Wednesday, April 26, 2023 at 9:48:04 AM UTC-7 Xiaohan Wang wrote:

I assumed that when mojo disconnection happens, the mojo async callback will be dropped on the thread/sequence where the `Remote` is bound to. +Ken Rockot to correct me if I am wrong. With that, the current `WrapCallbackWithDefaultInvokeIfNotRun` should suffice most use cases.

That would be most convenient indeed!   FWIW, I run some ad-hoc smoke tests where I've added a `CHECK(false)` to the utility-proces-hosted implementation of the mojo inteface and then verified that the fallback callback is still called on `DCHECK_CURRENTLY_ON(content::BrowserThread::UI)` at the following callstack:

qrcode_generator::QRImageGenerator::ForwardResponse()
base::internal::FunctorTraits<>::Invoke<>()
mojo::internal::CallbackWithDeleteHelper<>::Run()
base::internal::Invoker<>::RunOnce()
mojo::internal::CallbackWithDeleteHelper<>::~CallbackWithDeleteHelper()
base::internal::BindState<>::Destroy()
qrcode_generator::mojom::QRCodeGeneratorService_GenerateQRCode_ForwardToCallback::~QRCodeGeneratorService_GenerateQRCode_ForwardToCallback()
std::Cr::__tree<>::destroy()
mojo::InterfaceEndpointClient::NotifyError()
mojo::internal::MultiplexRouter::ProcessNotifyErrorTask()
mojo::internal::MultiplexRouter::ProcessTasks()
mojo::internal::MultiplexRouter::OnPipeConnectionError()
mojo::Connector::HandleError()
mojo::Connector::OnWatcherHandleReady()
base::RepeatingCallback<>::Run()
base::RepeatingCallback<>::Run()
mojo::SimpleWatcher::OnHandleReady()
base::internal::Invoker<>::RunOnce()
base::TaskAnnotator::RunTaskImpl()
base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWorkImpl()
base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork()
base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::DoWork()
base::MessagePumpGlib::Run()
base::sequence_manager::internal::ThreadControllerWithMessagePumpImpl::Run()
base::RunLoop::Run()
qrcode_generator::(anonymous namespace)::QrCodeGeneratorServicePixelTest::TestGolden()
qrcode_generator::(anonymous namespace)::QrCodeGeneratorServicePixelTest_DinoWithRoundQrPixelsAndLocators_Test::RunTestOnMainThread()

In the case where a callback may be dropped on a different thread, you can wrap the returned callback further with `base::BindPostTask`, where the input `callback` will always attempt to be destroyed on the target task runner:

https://source.chromium.org/chromium/chromium/src/+/main:base/task/bind_post_task.h;l=51-52

Best,

Xiaohan

On Wed, Apr 26, 2023 at 9:39 AM Łukasz Anforowicz <luk...@chromium.org> wrote:

Hello chromium-mojo@,

(Cc-ing petewil@ because the context of this question is the current QR Code Generator project;  Cc-ing gab@ because this question is somewhat related to TaskRunners)

Doc comments of WrapCallbackWithDefaultInvokeIfNotRun say that "the default form of the callback [...] may not run on the thread you expected" (i.e. the callback may be destructed on a different thread than the thread where Run is typically called).  In particular, I assume that mojo async response would typically be run on the same thread that the mojo::Remote is bound to (e.g. the UI thread), while the callback destruction may happen on the IO thread (where I assume mojo connectivity is managed).  Is there a recommended pattern or additional helpers to guarantee that the default form will be invoked on a specific thread (or sequence, or TaskRunner, or whatever the current technology is called :-))?

Initially I thought that maybe I can wrap `orignal_callback` (the first argument of WrapCallbackWithDefaultInvokeIfNotRun) with something like: base::BindOnce(PostToOriginalThread, std::move(original_callback), original_task_runner).  But then I've looked at base/threading/post_task_and_reply_impl.cc and it seems that calculating the `original_task_runner` may end up quite complicated.  Or... maybe it is okay to just get one through SequencedTaskRunner::GetCurrentDefault?

FWIW, the current callers of WrapCallbackWithDefaultInvokeIfNotRun (the few that I briefly looked at) seem to not care about the threading caveat (at least there are no comments that explain why the threading caveat doesn't apply + there are no threading-related wrappers).  Are these potential bugs?

Thanks,

Lukasz

--
You received this message because you are subscribed to the Google Groups "chromium-mojo" group.

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

[Daniel Cheng]

I don't actually remember the context behind that caveat. If your interface is bound on a sequence, disconnects will be received there; similarly, the reply callback will be dropped on the sequence where the Mojo endpoint is bound.

It /is/ better not to rely on WrapCallbackWithDefaultInvokeIfNotRun() if possible though; it can run at destruction time, when objects that your callback may have a reference to may also be in the process of being destroyed...

Daniel

To unsubscribe from this group and stop receiving emails from it, send an email to chromium-moj...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/chromium-mojo/5def02c7-d54c-4032-8388-b517b472ef96n%40chromium.org.