Disable thread-safe Send on ChannelMojo

Disable thread-safe Send on ChannelMojo

The world is not yet ready for thread-safe Send. D:

BUG=612944
TBR=jam@chromium.org

Committed: https://crrev.com/ff0d8038dd81bb025313a393cb8a6fcde9a79603
Cr-Commit-Position: refs/heads/master@{#394568}

[Ken Rockot(use gerrit already), 2016-05-18 20:50:47]

Description was changed from

==========
Disable thread-safe Send on ChannelMojo

The world is not yet ready for thread-safe Send.

BUG=612944
TBR=jam@chromium.org
==========

to

==========
Disable thread-safe Send on ChannelMojo

The world is not yet ready for thread-safe Send. D:

BUG=612944
TBR=jam@chromium.org
==========

[Ken Rockot(use gerrit already), 2016-05-18 20:52:06]

TBRing since it's trivial and semi-urgent with branch approaching.

[Ken Rockot(use gerrit already), 2016-05-18 20:53:28]

The CQ bit was checked by rockot@chromium.org

[commit-bot: I haz the power, 2016-05-18 20:54:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1991913003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1991913003/1

[commit-bot: I haz the power, 2016-05-18 22:03:48]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-18 22:05:02]

Description was changed from

==========
Disable thread-safe Send on ChannelMojo

The world is not yet ready for thread-safe Send. D:

BUG=612944
TBR=jam@chromium.org
==========

to

==========
Disable thread-safe Send on ChannelMojo

The world is not yet ready for thread-safe Send. D:

BUG=612944
TBR=jam@chromium.org

Committed: https://crrev.com/ff0d8038dd81bb025313a393cb8a6fcde9a79603
Cr-Commit-Position: refs/heads/master@{#394568}
==========

[commit-bot: I haz the power, 2016-05-18 22:05:03]

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

[gab, 2016-05-19 12:22:12]

gab@chromium.org changed reviewers:
+ gab@chromium.org

[gab, 2016-05-19 12:22:12]

Should we change the documentation on IPC::Channel::IsSendThreadSafe() to
highlight this problem that the codebase has overtime grown to depend on this
being false..?

[Ken Rockot(use gerrit already), 2016-05-19 13:24:36]

On May 19, 2016 5:22 AM, <gab@chromium.org> wrote:
>
> Should we change the documentation on IPC::Channel::IsSendThreadSafe() to
> highlight this problem that the codebase has overtime grown to depend on
this
> being false..?

Yeah, I'll update it once I figure out the right thing to do here. I'm a
bit torn between adding either a SendNow or a SendOnIOThread to
ChannelProxy.

SendNow would require all Channel impls to support it, and I don't see much
point in refactoring the legacy impl to be thread-safe.

On the other hand, a SendOnIOThread requires the effort of finding all the
bad assumptions and updating their call sites.

I suspect the vast majority of senders don't care, so I'm leaning toward
the latter.

>
> https://codereview.chromium.org/1991913003/

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

[gab, 2016-05-19 13:49:51]

On 2016/05/19 13:24:36, Ken Rockot wrote:
> On May 19, 2016 5:22 AM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote:
> >
> > Should we change the documentation on IPC::Channel::IsSendThreadSafe() to
> > highlight this problem that the codebase has overtime grown to depend on
> this
> > being false..?
> 
> Yeah, I'll update it once I figure out the right thing to do here. I'm a
> bit torn between adding either a SendNow or a SendOnIOThread to
> ChannelProxy.
> 
> SendNow would require all Channel impls to support it, and I don't see much
> point in refactoring the legacy impl to be thread-safe.
> 
> On the other hand, a SendOnIOThread requires the effort of finding all the
> bad assumptions and updating their call sites.
> 
> I suspect the vast majority of senders don't care, so I'm leaning toward
> the latter.

I suspect that too, but as Antoine stressed on cr-dev I think we'll have to
explicitly look at every single case and make an explicit decision. With that in
mind perhaps we want to deprecate Send() and introduce both SendNow() and
SendOnIOThread()? Migrating call sites one by one. The documentation on
SendNow() could be loose enough to let old impls do an IO thread bounce anyways
so that old impls don't have to change but callsites can make an explicit
decision about what they want their semantics to be.

This is the approach we'll be taking with the upcoming switch to
base/task_scheduler as well (mimic old implicit behaviours that most callers
don't actually depend on until they are migrated to TaskTraits where they'll
explicitly state what they depend on).

> 
> >
> > https://codereview.chromium.org/1991913003/
> 
> -- 
> 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
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....

[Ken Rockot(use gerrit already), 2016-05-19 14:20:21]

On 2016/05/19 at 13:49:51, gab wrote:
> On 2016/05/19 13:24:36, Ken Rockot wrote:
> > On May 19, 2016 5:22 AM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote:
> > >
> > > Should we change the documentation on IPC::Channel::IsSendThreadSafe() to
> > > highlight this problem that the codebase has overtime grown to depend on
> > this
> > > being false..?
> > 
> > Yeah, I'll update it once I figure out the right thing to do here. I'm a
> > bit torn between adding either a SendNow or a SendOnIOThread to
> > ChannelProxy.
> > 
> > SendNow would require all Channel impls to support it, and I don't see much
> > point in refactoring the legacy impl to be thread-safe.
> > 
> > On the other hand, a SendOnIOThread requires the effort of finding all the
> > bad assumptions and updating their call sites.
> > 
> > I suspect the vast majority of senders don't care, so I'm leaning toward
> > the latter.
> 
> I suspect that too, but as Antoine stressed on cr-dev I think we'll have to
explicitly look at every single case and make an explicit decision. With that in
mind perhaps we want to deprecate Send() and introduce both SendNow() and
SendOnIOThread()? Migrating call sites one by one. The documentation on
SendNow() could be loose enough to let old impls do an IO thread bounce anyways
so that old impls don't have to change but callsites can make an explicit
decision about what they want their semantics to be.

This SGTM. I'll put together a change soon.

> 
> This is the approach we'll be taking with the upcoming switch to
base/task_scheduler as well (mimic old implicit behaviours that most callers
don't actually depend on until they are migrated to TaskTraits where they'll
explicitly state what they depend on).
> 
> > 
> > >
> > > https://codereview.chromium.org/1991913003/
> > 
> > -- 
> > 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
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....

[Ken Rockot(use gerrit already), 2016-05-19 14:21:28]

On 2016/05/19 at 14:20:21, Ken Rockot wrote:
> On 2016/05/19 at 13:49:51, gab wrote:
> > On 2016/05/19 13:24:36, Ken Rockot wrote:
> > > On May 19, 2016 5:22 AM,
<https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote:
> > > >
> > > > Should we change the documentation on IPC::Channel::IsSendThreadSafe()
to
> > > > highlight this problem that the codebase has overtime grown to depend on
> > > this
> > > > being false..?
> > > 
> > > Yeah, I'll update it once I figure out the right thing to do here. I'm a
> > > bit torn between adding either a SendNow or a SendOnIOThread to
> > > ChannelProxy.
> > > 
> > > SendNow would require all Channel impls to support it, and I don't see
much
> > > point in refactoring the legacy impl to be thread-safe.
> > > 
> > > On the other hand, a SendOnIOThread requires the effort of finding all the
> > > bad assumptions and updating their call sites.
> > > 
> > > I suspect the vast majority of senders don't care, so I'm leaning toward
> > > the latter.
> > 
> > I suspect that too, but as Antoine stressed on cr-dev I think we'll have to
explicitly look at every single case and make an explicit decision. With that in
mind perhaps we want to deprecate Send() and introduce both SendNow() and
SendOnIOThread()? Migrating call sites one by one. The documentation on
SendNow() could be loose enough to let old impls do an IO thread bounce anyways
so that old impls don't have to change but callsites can make an explicit
decision about what they want their semantics to be.
> 
> This SGTM. I'll put together a change soon.

(also relevant: https://media.giphy.com/media/DZyxZgmcbC264/giphy.gif)

> 
> > 
> > This is the approach we'll be taking with the upcoming switch to
base/task_scheduler as well (mimic old implicit behaviours that most callers
don't actually depend on until they are migrated to TaskTraits where they'll
explicitly state what they depend on).
> > 
> > > 
> > > >
> > > > https://codereview.chromium.org/1991913003/
> > > 
> > > -- 
> > > 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
https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....