Mojo C++ bindings: fix Pause/Resume for MultiplexRouter.

Mojo C++ bindings: fix Pause/Resume for MultiplexRouter.

Previously if there are queued messages, MultiplexRouter may dispatch method
calls or connection error notifications while it is paused.

BUG=626869
Committed: https://crrev.com/0f89458416af0982a7d5708b6d99c54c31a8da0e
Cr-Commit-Position: refs/heads/master@{#406867}

[yzshen1, 2016-07-21 00:01:43]

The CQ bit was checked by yzshen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-21 00:02:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[yzshen1, 2016-07-21 00:03:35]

yzshen@chromium.org changed reviewers:
+ rockot@chromium.org, sky@chromium.org

[yzshen1, 2016-07-21 00:03:36]

Hi,

Ken: Would you please review?
Scott: FYI but welcome to comment too. Thanks!

I don't bother fixing Router because I wish to remove it some day and no one is
using its Pause/Resume.

[commit-bot: I haz the power, 2016-07-21 01:06:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-21 01:06:12]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-07-21 15:44:23]

friendly ping. :)

On Wed, Jul 20, 2016 at 5:03 PM, <yzshen@chromium.org> wrote:

> Reviewers: Ken Rockot, sky
> CL: https://codereview.chromium.org/2165153002/
>
> Message:
> Hi,
>
> Ken: Would you please review?
> Scott: FYI but welcome to comment too. Thanks!
>
> I don't bother fixing Router because I wish to remove it some day and no
> one is
> using its Pause/Resume.
>
>
>
> Description:
> Mojo C++ bindings: fix Pause/Resume for MultiplexRouter.
>
> Previously if there are queued messages, MultiplexRouter may dispatch
> method
> calls or connection error notifications while it is paused.
>
> BUG=626869
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+58, -24 lines):
> M mojo/public/cpp/bindings/lib/multiplex_router.h
> M mojo/public/cpp/bindings/lib/multiplex_router.cc
> M mojo/public/cpp/bindings/lib/router.h
>
>
> Index: mojo/public/cpp/bindings/lib/multiplex_router.cc
> diff --git a/mojo/public/cpp/bindings/lib/multiplex_router.cc
> b/mojo/public/cpp/bindings/lib/multiplex_router.cc
> index
>
dcfbab1545b4ef03eeac2bc6a2e365d972c7eb93..922cb6ce6111ae3ef702178457fb0671d969ad7b
> 100644
> --- a/mojo/public/cpp/bindings/lib/multiplex_router.cc
> +++ b/mojo/public/cpp/bindings/lib/multiplex_router.cc
> @@ -103,6 +103,20 @@ class MultiplexRouter::InterfaceEndpoint
> DCHECK_EQ(MOJO_RESULT_OK, result);
> }
>
> + void ResetSyncMessageSignal() {
> + router_->lock_.AssertAcquired();
> +
> + if (!event_signalled_)
> + return;
> +
> + DCHECK(sync_message_event_receiver_.is_valid());
> + MojoResult result =
> + ReadMessageRaw(sync_message_event_receiver_.get(), nullptr, nullptr,
> + nullptr, nullptr, MOJO_READ_MESSAGE_FLAG_MAY_DISCARD);
> + DCHECK_EQ(MOJO_RESULT_OK, result);
> + event_signalled_ = false;
> + }
> +
> //
> ---------------------------------------------------------------------------
> // The following public methods (i.e., InterfaceEndpointController
> // implementation) are called by the client on the same thread as the
> @@ -199,20 +213,6 @@ class MultiplexRouter::InterfaceEndpoint
> DCHECK_EQ(MOJO_RESULT_OK, result);
> }
>
> - void ResetSyncMessageSignal() {
> - router_->lock_.AssertAcquired();
> -
> - if (!event_signalled_)
> - return;
> -
> - DCHECK(sync_message_event_receiver_.is_valid());
> - MojoResult result = ReadMessageRaw(sync_message_event_receiver_.get(),
> - nullptr, nullptr, nullptr, nullptr,
> - MOJO_READ_MESSAGE_FLAG_MAY_DISCARD);
> - DCHECK_EQ(MOJO_RESULT_OK, result);
> - event_signalled_ = false;
> - }
> -
> //
> ---------------------------------------------------------------------------
> // The following members are safe to access from any threads.
>
> @@ -296,6 +296,7 @@ MultiplexRouter::MultiplexRouter(
> next_interface_id_value_(1),
> posted_to_process_tasks_(false),
> encountered_error_(false),
> + paused_(false),
> testing_mode_(false) {
> // Always participate in sync handle watching, because even if it doesn't
> // expect sync requests during sync handle watching, it may still need to
> @@ -456,6 +457,33 @@ void MultiplexRouter::CloseMessagePipe() {
> OnPipeConnectionError();
> }
>
> +void MultiplexRouter::PauseIncomingMethodCallProcessing() {
> + DCHECK(thread_checker_.CalledOnValidThread());
> + connector_.PauseIncomingMethodCallProcessing();
> +
> + base::AutoLock locker(lock_);
> + paused_ = true;
> +
> + for (auto iter = endpoints_.begin(); iter != endpoints_.end(); ++iter)
> + iter->second->ResetSyncMessageSignal();
> +}
> +
> +void MultiplexRouter::ResumeIncomingMethodCallProcessing() {
> + DCHECK(thread_checker_.CalledOnValidThread());
> + connector_.ResumeIncomingMethodCallProcessing();
> +
> + base::AutoLock locker(lock_);
> + paused_ = false;
> +
> + for (auto iter = endpoints_.begin(); iter != endpoints_.end(); ++iter) {
> + auto sync_iter = sync_message_tasks_.find(iter->first);
> + if (sync_iter != sync_message_tasks_.end() && !sync_iter->second.empty())
> + iter->second->SignalSyncMessageEvent();
> + }
> +
> + ProcessTasks(NO_DIRECT_CLIENT_CALLS, nullptr);
> +}
> +
> bool MultiplexRouter::HasAssociatedEndpoints() const {
> DCHECK(thread_checker_.CalledOnValidThread());
> base::AutoLock locker(lock_);
> @@ -482,6 +510,8 @@ bool MultiplexRouter::Accept(Message* message) {
> scoped_refptr<MultiplexRouter> protector(this);
> base::AutoLock locker(lock_);
>
> + DCHECK(!paused_);
> +
> ClientCallBehavior client_call_behavior =
> connector_.during_sync_handle_watcher_callback()
> ? ALLOW_DIRECT_CLIENT_CALLS_FOR_SYNC_MESSAGES
> @@ -590,7 +620,7 @@ void MultiplexRouter::ProcessTasks(
> if (posted_to_process_tasks_)
> return;
>
> - while (!tasks_.empty()) {
> + while (!tasks_.empty() && !paused_) {
> std::unique_ptr<Task> task(std::move(tasks_.front()));
> tasks_.pop_front();
>
> @@ -635,13 +665,16 @@ bool
> MultiplexRouter::ProcessFirstSyncMessageForEndpoint(InterfaceId id) {
> if (iter == sync_message_tasks_.end())
> return false;
>
> + if (paused_)
> + return true;
> +
> MultiplexRouter::Task* task = iter->second.front();
> iter->second.pop_front();
>
> DCHECK(task->IsMessageTask());
> std::unique_ptr<Message> message(std::move(task->message));
>
> - // Note: after this call, |task| and |iter| may be invalidated.
> + // Note: after this call, |task| and |iter| may be invalidated.
> bool processed = ProcessIncomingMessage(
> message.get(), ALLOW_DIRECT_CLIENT_CALLS_FOR_SYNC_MESSAGES, nullptr);
> DCHECK(processed);
> @@ -663,6 +696,8 @@ bool MultiplexRouter::ProcessNotifyErrorTask(
> ClientCallBehavior client_call_behavior,
> base::SingleThreadTaskRunner* current_task_runner) {
> DCHECK(!current_task_runner ||
> current_task_runner->BelongsToCurrentThread());
> + DCHECK(!paused_);
> +
> lock_.AssertAcquired();
> InterfaceEndpoint* endpoint = task->endpoint_to_notify.get();
> if (!endpoint->client())
> @@ -694,6 +729,7 @@ bool MultiplexRouter::ProcessIncomingMessage(
> ClientCallBehavior client_call_behavior,
> base::SingleThreadTaskRunner* current_task_runner) {
> DCHECK(!current_task_runner ||
> current_task_runner->BelongsToCurrentThread());
> + DCHECK(!paused_);
> lock_.AssertAcquired();
>
> if (!message) {
> Index: mojo/public/cpp/bindings/lib/multiplex_router.h
> diff --git a/mojo/public/cpp/bindings/lib/multiplex_router.h
> b/mojo/public/cpp/bindings/lib/multiplex_router.h
> index
>
dc66e8ee83230731ab42beb0e3bdbe3e2169b3b6..ccfd1c73dcb403e36ec3b72a481cd5d2100f27d5
> 100644
> --- a/mojo/public/cpp/bindings/lib/multiplex_router.h
> +++ b/mojo/public/cpp/bindings/lib/multiplex_router.h
> @@ -102,14 +102,8 @@ class MultiplexRouter
> }
>
> // See Binding for details of pause/resume.
> - void PauseIncomingMethodCallProcessing() {
> - DCHECK(thread_checker_.CalledOnValidThread());
> - connector_.PauseIncomingMethodCallProcessing();
> - }
> - void ResumeIncomingMethodCallProcessing() {
> - DCHECK(thread_checker_.CalledOnValidThread());
> - connector_.ResumeIncomingMethodCallProcessing();
> - }
> + void PauseIncomingMethodCallProcessing();
> + void ResumeIncomingMethodCallProcessing();
>
> // Whether there are any associated interfaces running currently.
> bool HasAssociatedEndpoints() const;
> @@ -229,6 +223,8 @@ class MultiplexRouter
>
> bool encountered_error_;
>
> + bool paused_;
> +
> bool testing_mode_;
>
> DISALLOW_COPY_AND_ASSIGN(MultiplexRouter);
> Index: mojo/public/cpp/bindings/lib/router.h
> diff --git a/mojo/public/cpp/bindings/lib/router.h
> b/mojo/public/cpp/bindings/lib/router.h
> index
>
6dbe08d5b20eac117984a794cb102dc9a93f3fad..d0c5dad0c38955d2b77eaf7c28bc67b06763bf76
> 100644
> --- a/mojo/public/cpp/bindings/lib/router.h
> +++ b/mojo/public/cpp/bindings/lib/router.h
> @@ -89,6 +89,8 @@ class Router : public MessageReceiverWithResponder {
> }
>
> // See Binding for details of pause/resume.
> + // Note: This doesn't strictly pause incoming calls. If there are
> + // queued messages, they may be dispatched during pause.
> void PauseIncomingMethodCallProcessing() {
> DCHECK(thread_checker_.CalledOnValidThread());
> connector_.PauseIncomingMethodCallProcessing();
>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2016-07-21 15:54:29]

Thanks for adding me, but Ken is the one that likely understands this code and
should do the review.

[Ken Rockot(use gerrit already), 2016-07-21 16:04:07]

lgtm

[yzshen1, 2016-07-21 16:11:59]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-07-21 16:12:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-21 16:15:54]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-07-21 16:17:49]

Description was changed from

==========
Mojo C++ bindings: fix Pause/Resume for MultiplexRouter.

Previously if there are queued messages, MultiplexRouter may dispatch method
calls or connection error notifications while it is paused.

BUG=626869
==========

to

==========
Mojo C++ bindings: fix Pause/Resume for MultiplexRouter.

Previously if there are queued messages, MultiplexRouter may dispatch method
calls or connection error notifications while it is paused.

BUG=626869

Committed: https://crrev.com/0f89458416af0982a7d5708b6d99c54c31a8da0e
Cr-Commit-Position: refs/heads/master@{#406867}
==========

[commit-bot: I haz the power, 2016-07-21 16:17:50]

Patchset 1 (id:??) landed as
https://crrev.com/0f89458416af0982a7d5708b6d99c54c31a8da0e
Cr-Commit-Position: refs/heads/master@{#406867}