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

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

BUG=634502
Committed: https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16
Cr-Commit-Position: refs/heads/master@{#418277}

[yzshen1, 2016-09-07 23:46:28]

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

[commit-bot: I haz the power, 2016-09-07 23:47:00]

Dry run: 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-09-07 23:50:46]

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

[commit-bot: I haz the power, 2016-09-07 23:50:47]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[yzshen1, 2016-09-07 23:54:43]

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

[yzshen1, 2016-09-07 23:54:44]

Hi, Ken.

Please take a look. What do you think about the interface exposed by this CL?

I am not quite sure whether this is sufficient. Imagine I receive an
InterfaceRequest<T>, I should be able to close it with a reason, without
explicitly bind it to an impl. That means I will need a more direct way to write
a control message into the pipe. In any case, I think that could be a follow-up
CL.

WDYT?

[Ken Rockot(use gerrit already), 2016-09-08 17:11:03]

I think developers might want to handle pipe closure reasons differently in
application logic (e.g. differentiate between a validation error, an explicit
closure, etc.), and that would be too fragile if we only used free-form strings
for reason.

Maybe we should try to come up with some basic useful enum of common reasons,
and allow for extensibility? Strawman proposal:

  enum DisconnectReason {
    USER_SPECIFIED,
    VALIDATION_ERROR,
    CLOSED
  };

  struct ConnectionError {
    DisconnectReason reason;
    int32 user_reason;
    string description;
  };

WDYT?

[yzshen1, 2016-09-08 21:24:43]

On 2016/09/08 17:11:03, Ken Rockot wrote:
> I think developers might want to handle pipe closure reasons differently in
> application logic (e.g. differentiate between a validation error, an explicit
> closure, etc.), and that would be too fragile if we only used free-form
strings
> for reason.
> 
> Maybe we should try to come up with some basic useful enum of common reasons,
> and allow for extensibility? Strawman proposal:
> 
>   enum DisconnectReason {
>     USER_SPECIFIED,
>     VALIDATION_ERROR,
>     CLOSED
>   };
> 
>   struct ConnectionError {
>     DisconnectReason reason;
>     int32 user_reason;
>     string description;
>   };
> 
> WDYT?

I think VALIDATION_ERROR shouldn't be needed: a well-behaved sender should never
trigger a validation error, it is fully preventable from the sender side.
What is the difference between CLOSED and USER_SPECIFIED?

I have been thinking about this back-and-forth, I will give it more thoughts.
Thanks!

[Fady Samuel, 2016-09-08 21:29:40]

On 2016/09/08 21:24:43, yzshen1 wrote:
> On 2016/09/08 17:11:03, Ken Rockot wrote:
> > I think developers might want to handle pipe closure reasons differently in
> > application logic (e.g. differentiate between a validation error, an
explicit
> > closure, etc.), and that would be too fragile if we only used free-form
> strings
> > for reason.
> > 
> > Maybe we should try to come up with some basic useful enum of common
reasons,
> > and allow for extensibility? Strawman proposal:
> > 
> >   enum DisconnectReason {
> >     USER_SPECIFIED,
> >     VALIDATION_ERROR,
> >     CLOSED
> >   };
> > 
> >   struct ConnectionError {
> >     DisconnectReason reason;
> >     int32 user_reason;
> >     string description;
> >   };
> > 
> > WDYT?
> 
> I think VALIDATION_ERROR shouldn't be needed: a well-behaved sender should
never
> trigger a validation error, it is fully preventable from the sender side.
> What is the difference between CLOSED and USER_SPECIFIED?
> 
> I have been thinking about this back-and-forth, I will give it more thoughts.
> Thanks!

Drive by thought: I might be missing context and maybe my suggestion is
meaningless but I think knowing validation error is valuable because in that
case, the service (browser process potentially) might want to kill the client in
response to a validation error. In other words, I don't think we should assume
the sender is well behaved?

[yzshen1, 2016-09-08 21:33:46]

On 2016/09/08 21:29:40, Fady Samuel wrote:
> On 2016/09/08 21:24:43, yzshen1 wrote:
> > On 2016/09/08 17:11:03, Ken Rockot wrote:
> > > I think developers might want to handle pipe closure reasons differently
in
> > > application logic (e.g. differentiate between a validation error, an
> explicit
> > > closure, etc.), and that would be too fragile if we only used free-form
> > strings
> > > for reason.
> > > 
> > > Maybe we should try to come up with some basic useful enum of common
> reasons,
> > > and allow for extensibility? Strawman proposal:
> > > 
> > >   enum DisconnectReason {
> > >     USER_SPECIFIED,
> > >     VALIDATION_ERROR,
> > >     CLOSED
> > >   };
> > > 
> > >   struct ConnectionError {
> > >     DisconnectReason reason;
> > >     int32 user_reason;
> > >     string description;
> > >   };
> > > 
> > > WDYT?
> > 
> > I think VALIDATION_ERROR shouldn't be needed: a well-behaved sender should
> never
> > trigger a validation error, it is fully preventable from the sender side.
> > What is the difference between CLOSED and USER_SPECIFIED?
> > 
> > I have been thinking about this back-and-forth, I will give it more
thoughts.
> > Thanks!
> 
> Drive by thought: I might be missing context and maybe my suggestion is
> meaningless but I think knowing validation error is valuable because in that
> case, the service (browser process potentially) might want to kill the client
in
> response to a validation error. In other words, I don't think we should assume
> the sender is well behaved?

Thanks for input, Fady!

This is a message that one can choose to send to the other side of the message
pipe, when deciding to close the message pipe.
If I decide to close a message pipe because the sender sends me an invalid
message, I don't really need to explain the reason to the sender.
On the other hand, a well-behave sender should never trigger a validation error,
so it doesn't need to care about such a message, either.

I agree we shouldn't assume the sender is well behaved, and didn't make that
assumption here. :)

Thanks!

[Ken Rockot(use gerrit already), 2016-09-08 21:38:09]

We already have a mechanism for reporting validation errors to the EDK
embedder in a way that easily allows the embedder to handle that sort of
kill behavior on a process-wide level. We shouldn't expect interface impls
to handle that sort of thing individually.

I agree with your rationale Yuzhu, a validation error doesn't make much
sense, so we could avoid introducing a common enum. However I think there
is still value in supporting a user-specified error code of sorts.
Something which is cheap and compact and easy to compare. My only concern
is the awkwardness of using something like a generic integer that users
will cast between that and a custom enum, but I think this is still less
unfortunate than doing string comparisons everywhere. WDYT?

On Thu, Sep 8, 2016 at 2:33 PM, <yzshen@chromium.org> wrote:

> On 2016/09/08 21:29:40, Fady Samuel wrote:
> > On 2016/09/08 21:24:43, yzshen1 wrote:
> > > On 2016/09/08 17:11:03, Ken Rockot wrote:
> > > > I think developers might want to handle pipe closure reasons
> differently
> in
> > > > application logic (e.g. differentiate between a validation error, an
> > explicit
> > > > closure, etc.), and that would be too fragile if we only used
> free-form
> > > strings
> > > > for reason.
> > > >
> > > > Maybe we should try to come up with some basic useful enum of common
> > reasons,
> > > > and allow for extensibility? Strawman proposal:
> > > >
> > > > enum DisconnectReason {
> > > > USER_SPECIFIED,
> > > > VALIDATION_ERROR,
> > > > CLOSED
> > > > };
> > > >
> > > > struct ConnectionError {
> > > > DisconnectReason reason;
> > > > int32 user_reason;
> > > > string description;
> > > > };
> > > >
> > > > WDYT?
> > >
> > > I think VALIDATION_ERROR shouldn't be needed: a well-behaved sender
> should
> > never
> > > trigger a validation error, it is fully preventable from the sender
> side.
> > > What is the difference between CLOSED and USER_SPECIFIED?
> > >
> > > I have been thinking about this back-and-forth, I will give it more
> thoughts.
> > > Thanks!
> >
> > Drive by thought: I might be missing context and maybe my suggestion is
> > meaningless but I think knowing validation error is valuable because in
> that
> > case, the service (browser process potentially) might want to kill the
> client
> in
> > response to a validation error. In other words, I don't think we should
> assume
> > the sender is well behaved?
>
> Thanks for input, Fady!
>
> This is a message that one can choose to send to the other side of the
> message
> pipe, when deciding to close the message pipe.
> If I decide to close a message pipe because the sender sends me an invalid
> message, I don't really need to explain the reason to the sender.
> On the other hand, a well-behave sender should never trigger a validation
> error,
> so it doesn't need to care about such a message, either.
>
> I agree we shouldn't assume the sender is well behaved, and didn't make
> that
> assumption here. :)
>
> Thanks!
>
> https://codereview.chromium.org/2318793002/
>

-- 
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.

[Fady Samuel, 2016-09-08 21:40:32]

Thanks Yuzhu, Ken,

Apologies for the confusion! Ignore my comment then!

Fady

On 2016/09/08 21:38:09, Ken Rockot wrote:
> We already have a mechanism for reporting validation errors to the EDK
> embedder in a way that easily allows the embedder to handle that sort of
> kill behavior on a process-wide level. We shouldn't expect interface impls
> to handle that sort of thing individually.
> 
> I agree with your rationale Yuzhu, a validation error doesn't make much
> sense, so we could avoid introducing a common enum. However I think there
> is still value in supporting a user-specified error code of sorts.
> Something which is cheap and compact and easy to compare. My only concern
> is the awkwardness of using something like a generic integer that users
> will cast between that and a custom enum, but I think this is still less
> unfortunate than doing string comparisons everywhere. WDYT?
> 
> On Thu, Sep 8, 2016 at 2:33 PM, <mailto:yzshen@chromium.org> wrote:
> 
> > On 2016/09/08 21:29:40, Fady Samuel wrote:
> > > On 2016/09/08 21:24:43, yzshen1 wrote:
> > > > On 2016/09/08 17:11:03, Ken Rockot wrote:
> > > > > I think developers might want to handle pipe closure reasons
> > differently
> > in
> > > > > application logic (e.g. differentiate between a validation error, an
> > > explicit
> > > > > closure, etc.), and that would be too fragile if we only used
> > free-form
> > > > strings
> > > > > for reason.
> > > > >
> > > > > Maybe we should try to come up with some basic useful enum of common
> > > reasons,
> > > > > and allow for extensibility? Strawman proposal:
> > > > >
> > > > > enum DisconnectReason {
> > > > > USER_SPECIFIED,
> > > > > VALIDATION_ERROR,
> > > > > CLOSED
> > > > > };
> > > > >
> > > > > struct ConnectionError {
> > > > > DisconnectReason reason;
> > > > > int32 user_reason;
> > > > > string description;
> > > > > };
> > > > >
> > > > > WDYT?
> > > >
> > > > I think VALIDATION_ERROR shouldn't be needed: a well-behaved sender
> > should
> > > never
> > > > trigger a validation error, it is fully preventable from the sender
> > side.
> > > > What is the difference between CLOSED and USER_SPECIFIED?
> > > >
> > > > I have been thinking about this back-and-forth, I will give it more
> > thoughts.
> > > > Thanks!
> > >
> > > Drive by thought: I might be missing context and maybe my suggestion is
> > > meaningless but I think knowing validation error is valuable because in
> > that
> > > case, the service (browser process potentially) might want to kill the
> > client
> > in
> > > response to a validation error. In other words, I don't think we should
> > assume
> > > the sender is well behaved?
> >
> > Thanks for input, Fady!
> >
> > This is a message that one can choose to send to the other side of the
> > message
> > pipe, when deciding to close the message pipe.
> > If I decide to close a message pipe because the sender sends me an invalid
> > message, I don't really need to explain the reason to the sender.
> > On the other hand, a well-behave sender should never trigger a validation
> > error,
> > so it doesn't need to care about such a message, either.
> >
> > I agree we shouldn't assume the sender is well behaved, and didn't make
> > that
> > assumption here. :)
> >
> > Thanks!
> >
> > https://codereview.chromium.org/2318793002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2016-09-08 21:51:35]

On 2016/09/08 21:38:09, Ken Rockot wrote:
> We already have a mechanism for reporting validation errors to the EDK
> embedder in a way that easily allows the embedder to handle that sort of
> kill behavior on a process-wide level. We shouldn't expect interface impls
> to handle that sort of thing individually.
> 
> I agree with your rationale Yuzhu, a validation error doesn't make much
> sense, so we could avoid introducing a common enum. However I think there
> is still value in supporting a user-specified error code of sorts.
> Something which is cheap and compact and easy to compare. My only concern
> is the awkwardness of using something like a generic integer that users
> will cast between that and a custom enum, but I think this is still less
> unfortunate than doing string comparisons everywhere. WDYT?

That makes sense.
What do you think about
{
  uint32_t custom_reason;  // The same type as MojoResult, but I don't really
have strong preference.
  string? description;
}


> 
> On Thu, Sep 8, 2016 at 2:33 PM, <mailto:yzshen@chromium.org> wrote:
> 
> > On 2016/09/08 21:29:40, Fady Samuel wrote:
> > > On 2016/09/08 21:24:43, yzshen1 wrote:
> > > > On 2016/09/08 17:11:03, Ken Rockot wrote:
> > > > > I think developers might want to handle pipe closure reasons
> > differently
> > in
> > > > > application logic (e.g. differentiate between a validation error, an
> > > explicit
> > > > > closure, etc.), and that would be too fragile if we only used
> > free-form
> > > > strings
> > > > > for reason.
> > > > >
> > > > > Maybe we should try to come up with some basic useful enum of common
> > > reasons,
> > > > > and allow for extensibility? Strawman proposal:
> > > > >
> > > > > enum DisconnectReason {
> > > > > USER_SPECIFIED,
> > > > > VALIDATION_ERROR,
> > > > > CLOSED
> > > > > };
> > > > >
> > > > > struct ConnectionError {
> > > > > DisconnectReason reason;
> > > > > int32 user_reason;
> > > > > string description;
> > > > > };
> > > > >
> > > > > WDYT?
> > > >
> > > > I think VALIDATION_ERROR shouldn't be needed: a well-behaved sender
> > should
> > > never
> > > > trigger a validation error, it is fully preventable from the sender
> > side.
> > > > What is the difference between CLOSED and USER_SPECIFIED?
> > > >
> > > > I have been thinking about this back-and-forth, I will give it more
> > thoughts.
> > > > Thanks!
> > >
> > > Drive by thought: I might be missing context and maybe my suggestion is
> > > meaningless but I think knowing validation error is valuable because in
> > that
> > > case, the service (browser process potentially) might want to kill the
> > client
> > in
> > > response to a validation error. In other words, I don't think we should
> > assume
> > > the sender is well behaved?
> >
> > Thanks for input, Fady!
> >
> > This is a message that one can choose to send to the other side of the
> > message
> > pipe, when deciding to close the message pipe.
> > If I decide to close a message pipe because the sender sends me an invalid
> > message, I don't really need to explain the reason to the sender.
> > On the other hand, a well-behave sender should never trigger a validation
> > error,
> > so it doesn't need to care about such a message, either.
> >
> > I agree we shouldn't assume the sender is well behaved, and didn't make
> > that
> > assumption here. :)
> >
> > Thanks!
> >
> > https://codereview.chromium.org/2318793002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2016-09-12 20:10:58]

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

[commit-bot: I haz the power, 2016-09-12 20:11:35]

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

[yzshen1, 2016-09-12 20:13:33]

Hi, Ken.

I have updated the CL to have a uint32_t reason code and a string description.
PTAL. Thanks!

[Ken Rockot(use gerrit already), 2016-09-12 21:02:26]

lgtm

[commit-bot: I haz the power, 2016-09-12 21:40:58]

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

[commit-bot: I haz the power, 2016-09-12 21:40:59]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-09-12 22:18:50]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2016-09-12 22:19:31]

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

[yzshen1, 2016-09-12 22:21:16]

yzshen@chromium.org changed reviewers:
+ dcheng@chromium.org

[yzshen1, 2016-09-12 22:21:17]

Hi, Daniel.

Would you please take a look at the changes to mojom? Thanks!

[commit-bot: I haz the power, 2016-09-12 22:27:36]

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

[commit-bot: I haz the power, 2016-09-12 22:27:37]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dcheng, 2016-09-13 00:59:49]

lgtm

[yzshen1, 2016-09-13 15:42:28]

The CQ bit was checked by yzshen@chromium.org

[yzshen1, 2016-09-13 15:42:29]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2318793002/#ps100001
(title: ".")

[commit-bot: I haz the power, 2016-09-13 15:42:44]

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-09-13 17:04:11]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-09-13 17:07:28]

Description was changed from

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

BUG=634502
==========

to

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

BUG=634502

Committed: https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16
Cr-Commit-Position: refs/heads/master@{#418277}
==========

[commit-bot: I haz the power, 2016-09-13 17:07:30]

Patchset 6 (id:??) landed as
https://crrev.com/24438d2d601245d80c2bf9e40a07e7f3c9a10e16
Cr-Commit-Position: refs/heads/master@{#418277}