New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 634502 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Support specifying a reason for pipe disconnection

Project Member Reported by yzshen@chromium.org, Aug 4 2016

Issue description

This was a suggestion from Darin with the following use case:

When the browser receives too many websocket interface requests in a brief period of time, it may want to close some of the pipes and let the renderers know that they are closed because of throttling. However, currently there isn't a good way to convey the reason. One possible, but clumsy way is that the browser waits for a client interface on the pipe it wants to disconnect; makes a call on the client interface to tell the reason; close both pipes.

Two possible ways to support this: (1) we could make it a system API; (2) we could make it a binding-level feature, by issuing a control message that is sent right before the pipe is disconnected.

#2 seems to be a better choice, because this way we can also support disconnection reason for associated interfaces, which don't have their own message pipes.

WDYT?
 
I like the idea of using a control message
Owner: yzshen@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/24438d2d601245d80c2bf9e40a07e7f3c9a10e16

commit 24438d2d601245d80c2bf9e40a07e7f3c9a10e16
Author: yzshen <yzshen@chromium.org>
Date: Tue Sep 13 17:03:53 2016

Mojo C++ bindings: support disconnect with a reason.

BUG= 634502 

Review-Url: https://codereview.chromium.org/2318793002
Cr-Commit-Position: refs/heads/master@{#418277}

[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/BUILD.gn
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/associated_binding.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/associated_interface_ptr.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/binding.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/binding_set.h
[add] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/connection_error_callback.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/interface_endpoint_client.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/interface_ptr.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/associated_interface_ptr_state.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/binding_state.cc
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/binding_state.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/control_message_handler.cc
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/control_message_handler.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/control_message_proxy.cc
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/control_message_proxy.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/interface_endpoint_client.cc
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/interface_ptr_state.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/router.cc
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/lib/router.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/strong_binding.h
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/tests/associated_interface_unittest.cc
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/tests/binding_set_unittest.cc
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/tests/binding_unittest.cc
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/cpp/bindings/tests/interface_ptr_unittest.cc
[modify] https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16/mojo/public/interfaces/bindings/interface_control_messages.mojom

Comment 5 by m...@chromium.org, Sep 21 2016

I was pointed at this new feature recently, but I'm a bit confused as to why the implementation is the way it is: In particular, I'm concerned how the code at either end of a pipe could guarantee the values passed via "uint32_t custom_reason" are treated semantically the same? In other words, there is no strong type checking around this value.

As a suggestion: Perhaps there should be some mojom definition to provide an enum for the valid set of "close reasons," and then some code-gen, such as new accessors in InterfacePtr and InterfaceRequest, to provide access that close reason. Then, the close reason would be strongly-typed around that enum. WDYT?

OTOH, an interface definition might look like:

enum FooCloseReasons {
  BECAUSE_I_SAID_SO,
  TOO_MANY_WEBSOCKETS,
  SOMETHING_ELSE,
};

interface Foo {
  binding {
    at_close = FooCloseReasons;
  }

  DoSomething();
};


Comment 6 by m...@chromium.org, Sep 21 2016

Cc: m...@chromium.org

Comment 7 by roc...@chromium.org, Sep 21 2016

I think that would be a reasonable way to use the control message. We could
implement strongly typed error code support using attribute syntax:

[ConnectionErrorType=FooCloseReasons]
interface Foo { ... }

Comment 8 by yzshen@chromium.org, Sep 21 2016

At the moment, I am using const values defined inside the interface, such as:
https://codereview.chromium.org/2343433002/diff/1/third_party/WebKit/public/platform/modules/websockets/websocket.mojom

I think your idea of using strongly typed enum is doable and could be done as a follow-up improvement.

Thanks for the input!

Cc: ben@chromium.org
 Issue 587183  has been merged into this issue.
Components: -Internals>Mojo Internals>Mojo>Bindings
Status: Fixed (was: Started)

Sign in to add a comment