Do not mark alternate-protocol broken unless the main job succeeds.

Do not mark alternate-protocol broken unless the main job succeeds.

Currently, if both the main and alternate jobs fail, we mark alternate-protocol as broken. However, that's not really "fair" to the alternate-protocol. We should only mark it as broken in the alternate-protocol jobs fails and the main job succeeds.

This is a bit tricky because the two jobs may complete in any order.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271994

[Ryan Hamilton, 2014-04-22 21:39:02]

Hi Will,

I think your the most familiar with the Alternate-Protocol code.  Can you take a
look at this CL?

Thanks!

[willchan no longer on Chromium, 2014-04-22 21:46:11]

Starting the review. Is there a bug report for this? I kinda feel like I must be
misunderstanding the issue, because we race all the time, and I don't think that
the main job winning the race will mark the Alternate-Protocol as broken. But
I'll go read the code :)

https://codereview.chromium.org/245663003/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/245663003/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.h:99: // Marks that the other job |job|
has completed.
Reads a little weird, I think it'd be better to delete the job prior to |job|.

[Ryan Hamilton, 2014-04-22 21:53:36]

On 2014/04/22 21:46:11, willchan wrote:
> Starting the review. Is there a bug report for this? I kinda feel like I must
be
> misunderstanding the issue, because we race all the time, and I don't think
that
> the main job winning the race will mark the Alternate-Protocol as broken. But
> I'll go read the code :)

Sorry, what I meant to say was:

Currently, if both the main and alternate jobs fail, we mark alternate-protocol
as broken. However, that's not really "fair" to the alternate-protocol. We
should only mark it as broken in the alternate-protocol jobs fails and the main
job succeeds.

I'll update the CL description.  (No bug for this)

[Ryan Hamilton, 2014-04-22 21:53:38]

On 2014/04/22 21:46:11, willchan wrote:
> Starting the review. Is there a bug report for this? I kinda feel like I must
be
> misunderstanding the issue, because we race all the time, and I don't think
that
> the main job winning the race will mark the Alternate-Protocol as broken. But
> I'll go read the code :)

Sorry, what I meant to say was:

Currently, if both the main and alternate jobs fail, we mark alternate-protocol
as broken. However, that's not really "fair" to the alternate-protocol. We
should only mark it as broken in the alternate-protocol jobs fails and the main
job succeeds.

I'll update the CL description.  (No bug for this)

[Ryan Hamilton, 2014-04-22 21:54:44]

https://codereview.chromium.org/245663003/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/245663003/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.h:99: // Marks that the other job |job|
has completed.
On 2014/04/22 21:46:11, willchan wrote:
> Reads a little weird, I think it'd be better to delete the job prior to |job|.

What do you mean by "delete the job prior to job"?

[Ryan Hamilton, 2014-04-22 21:57:45]

https://codereview.chromium.org/245663003/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/245663003/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.h:99: // Marks that the other job |job|
has completed.
On 2014/04/22 21:54:44, Ryan Hamilton wrote:
> On 2014/04/22 21:46:11, willchan wrote:
> > Reads a little weird, I think it'd be better to delete the job prior to
|job|.
> 
> What do you mean by "delete the job prior to job"?

Oh, I see. Done.

[willchan no longer on Chromium, 2014-04-22 22:23:54]

Getting a headache trying to figure out which design I prefer :)

https://codereview.chromium.org/245663003/diff/40001/net/http/http_stream_fac...
File net/http/http_stream_factory_impl_job.h (right):

https://codereview.chromium.org/245663003/diff/40001/net/http/http_stream_fac...
net/http/http_stream_factory_impl_job.h:346: JobStatus job_status_;
At some point I think we need to remove the Alternate-Protocol logic from the
job and move it over to the HttpStreamFactoryImpl or the Request. I kinda feel
like a Job should not have to know that it's the "alternate". It should just run
to completion, and then notify some other object that it is done.

I'm still kicking around ideas in my head, but here are some things that come to
mind:
* Move the knowledge of the "alternate" to the Request or Factory
* Switch the Job to being an interface with a JobImpl as the base
implementation. Compose this base implementation with some other object that
understands the Alternate-Protocol relationship.

[willchan no longer on Chromium, 2014-04-22 22:38:24]

After thinking about it a bit more, I'm becoming convinced we need a
"JobController" type of object. The Request communicates with the JobController,
which communicates with the various jobs. The JobController mostly passes the
messages along between the request and jobs, but might update the
HttpServerProperties. The reason we can't just merge this controller logic
directly into the Request is because its lifetime is tied to the request usage,
not the jobs. We need something whose lifetime is tied to all the jobs.
Currently you achieve that by a distributed p2p protocol between each job, but I
think it'll be cleaner / easier to understand if we create a controller object
instead.

I also looked at the job orphaning logic here. I need to page it back in, but I
think it still does mostly the right thing. But it's important not to completely
detach the job from the ownership graph, because then we can't cancel on
shutdown. Ditto with the controller. The factory probably needs to own all
controllers.

Note that what I outlined is what I believe to be the "right" way forward here.
I don't know if now is the right time for such a yak shave. I just wanted to
document it so we could both agree on a future direction. If you need to do
other stuff first, it's fine to just document this with TODOs and what not.

[Ryan Hamilton, 2014-04-23 01:36:15]

On 2014/04/22 22:38:24, willchan wrote:
> After thinking about it a bit more, I'm becoming convinced we need a
> "JobController" type of object. The Request communicates with the
JobController,
> which communicates with the various jobs. The JobController mostly passes the
> messages along between the request and jobs, but might update the
> HttpServerProperties. The reason we can't just merge this controller logic
> directly into the Request is because its lifetime is tied to the request
usage,
> not the jobs. We need something whose lifetime is tied to all the jobs.
> Currently you achieve that by a distributed p2p protocol between each job, but
I
> think it'll be cleaner / easier to understand if we create a controller object
> instead.
> 
> I also looked at the job orphaning logic here. I need to page it back in, but
I
> think it still does mostly the right thing. But it's important not to
completely
> detach the job from the ownership graph, because then we can't cancel on
> shutdown. Ditto with the controller. The factory probably needs to own all
> controllers.
> 
> Note that what I outlined is what I believe to be the "right" way forward
here.
> I don't know if now is the right time for such a yak shave. I just wanted to
> document it so we could both agree on a future direction. If you need to do
> other stuff first, it's fine to just document this with TODOs and what not.

Ooo! Yes, I like this quite a bit. In working on this CL, it was annoying that
there was no object that was sitting in a place where it could understand the
status of both jobs. A controller which owns the jobs sounds like the way to go.
I guess it would be owned by the Factory, and referenced from the Request (which
is in turn owned by the Factory, right?). I'm happy to do this... I think it
would be a nice cleanup.

Let me see how much work it would be to go this route. I may well take you up on
the offer to land this CL with TODOs.

[Ryan Hamilton, 2014-05-20 03:11:57]

On 2014/04/23 01:36:15, Ryan Hamilton wrote:
> 
> Let me see how much work it would be to go this route. I may well take you up
on
> the offer to land this CL with TODOs.

I'd really love to get this feature landed. Would you mind if we landed this
while we work on getting the JobController CL squared away?

[willchan no longer on Chromium, 2014-05-20 16:33:34]

That's fine by me.


On Mon, May 19, 2014 at 8:11 PM, <rch@chromium.org> wrote:

> On 2014/04/23 01:36:15, Ryan Hamilton wrote:
>
>  Let me see how much work it would be to go this route. I may well take
>> you up
>>
> on
>
>> the offer to land this CL with TODOs.
>>
>
> I'd really love to get this feature landed. Would you mind if we landed
> this
> while we work on getting the JobController CL squared away?
>
> https://codereview.chromium.org/245663003/
>

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

[Ryan Hamilton, 2014-05-20 17:12:19]

On 2014/05/20 16:33:34, willchan wrote:
> That's fine by me.

Can you LGTM this, then? :>

[willchan no longer on Chromium, 2014-05-20 17:14:55]

lgtm

[Ryan Hamilton, 2014-05-20 17:38:09]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-05-20 17:39:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/245663003/60001

[commit-bot: I haz the power, 2014-05-20 19:34:43]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-05-20 19:39:22]

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

[commit-bot: I haz the power, 2014-05-20 19:39:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[Ryan Hamilton, 2014-05-20 23:01:39]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-05-20 23:03:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/245663003/80001

[commit-bot: I haz the power, 2014-05-21 07:26:59]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-21 07:33:18]

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

[commit-bot: I haz the power, 2014-05-21 07:33:19]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[jar (doing other things), 2014-05-21 16:03:25]

histograms.xml LGTM

[jar (doing other things), 2014-05-21 16:05:33]

The CQ bit was checked by jar@chromium.org

[Ryan Hamilton, 2014-05-21 18:52:22]

The CQ bit was unchecked by rch@chromium.org

[Ryan Hamilton, 2014-05-21 18:52:24]

The CQ bit was checked by rch@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:57:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rch@chromium.org/245663003/80001

[commit-bot: I haz the power, 2014-05-21 22:38:28]

Change committed as 271994