Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1121043002:
Make alternate Jobs not use HTTP/1.1 sockets. (Closed)
DescriptionMake alternate Jobs not use HTTP/1.1 sockets.
An alternate HttpStreamFactoryImpy::Job should not use a socket if HTTP/1.1 is negotiated, nor should it pool to an already open HTTP/1.1 socket.
Patch Set 1: Test 7 per https://crbug.com/474217#c6. This should pass.
Patch Set 2: Test 8 per https://crbug.com/474217#c6. This will fail.
Patch Set 3: One possible fix for the failing test.
Patch Set 4: Another possible fix for the failing test.
BUG=474217
Committed: https://crrev.com/5452e2a52b71e3a2a4c471c864155831a9a59951
Cr-Commit-Position: refs/heads/master@{#328964}
Patch Set 1 #Patch Set 2 : Add failing test. #Patch Set 3 : One possible fix. #Patch Set 4 : Another possible fix. #
Total comments: 15
Patch Set 5 : Re: #3. #Patch Set 6 : Re: #5. #
Total comments: 2
Patch Set 7 : Re: #8. #
Total comments: 13
Patch Set 8 : Re: #10. #Patch Set 9 : Re: #13. #
Messages
Total messages: 20 (5 generated)
bnc@chromium.org changed reviewers: + davidben@chromium.org
David: PTAL. Thanks.
lgtm https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12015: rv = callback.WaitForResult(); Since we don't actually care whether Start completes synchronously or no, I would write: EXPECT_EQ(ERR_NPN_NEGOTIATED_FAILED, callback.GetResult(trans->Start(..., callback.callback(), ...)); Ditto elsewhere. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12090: ASSERT_TRUE(response1 != nullptr); Nit: I think this can just be ASSERT_TRUE(response1) https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12091: ASSERT_TRUE(response1->headers.get() != nullptr); Nit: Ditto. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12117: // socket is still open and in the pool. Huh, interesting. I guess this works because we don't explicitly Disconnect() the socket and the default behavior must be to return it. So then the thing about draining the socket pool we were talking about doesn't really happen. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12132: ASSERT_TRUE(response3->headers.get() != nullptr); Ditto. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1121043002/diff/60001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:861: bool want_spdy_over_npn = IsAlternate(); (I'm guessing this is to be cleaned up in a follow-up?) https://codereview.chromium.org/1121043002/diff/60001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1116: } Nit: While I do like curly braces, Chromium style is to omit them.
David: PTAL at how I remove EXPECT(trans->Start()). Thanks. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12015: rv = callback.WaitForResult(); On 2015/05/01 23:21:46, David Benjamin wrote: > Since we don't actually care whether Start completes synchronously or no, I > would write: > > EXPECT_EQ(ERR_NPN_NEGOTIATED_FAILED, callback.GetResult(trans->Start(..., > callback.callback(), ...)); > > Ditto elsewhere. I'm not a big fan multiply nested function calls or very long lines. How about this instead? Also note that because of the way git cl format breaks line, what I propose is two fewer lines in total. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12090: ASSERT_TRUE(response1 != nullptr); On 2015/05/01 23:21:46, David Benjamin wrote: > Nit: I think this can just be ASSERT_TRUE(response1) Done. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12091: ASSERT_TRUE(response1->headers.get() != nullptr); On 2015/05/01 23:21:46, David Benjamin wrote: > Nit: Ditto. Done. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12117: // socket is still open and in the pool. On 2015/05/01 23:21:46, David Benjamin wrote: > Huh, interesting. I guess this works because we don't explicitly Disconnect() > the socket and the default behavior must be to return it. So then the thing > about draining the socket pool we were talking about doesn't really happen. Yup. I think this is really essential for this test here, otherwise there is no way to know if request2 didn't pool to the first socket just because that had gotten disconnected already. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12132: ASSERT_TRUE(response3->headers.get() != nullptr); On 2015/05/01 23:21:46, David Benjamin wrote: > Ditto. Done. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_stream_fa... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1121043002/diff/60001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:861: bool want_spdy_over_npn = IsAlternate(); On 2015/05/01 23:21:46, David Benjamin wrote: > (I'm guessing this is to be cleaned up in a follow-up?) Yes, that's the plan. https://codereview.chromium.org/1121043002/diff/60001/net/http/http_stream_fa... net/http/http_stream_factory_impl_job.cc:1116: } On 2015/05/01 23:21:46, David Benjamin wrote: > Nit: While I do like curly braces, Chromium style is to omit them. Done.
https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1121043002/diff/60001/net/http/http_network_t... net/http/http_network_transaction_unittest.cc:12015: rv = callback.WaitForResult(); On 2015/05/04 12:05:07, Bence wrote: > On 2015/05/01 23:21:46, David Benjamin wrote: > > Since we don't actually care whether Start completes synchronously or no, I > > would write: > > > > EXPECT_EQ(ERR_NPN_NEGOTIATED_FAILED, callback.GetResult(trans->Start(..., > > callback.callback(), ...)); > > > > Ditto elsewhere. > > I'm not a big fan multiply nested function calls or very long lines. How about > this instead? Also note that because of the way git cl format breaks line, what > I propose is two fewer lines in total. That'll silently break if things, for some reason, start completing synchronously. I think the old version is preferable there since the EXPECT_EQ (probably better an ASSERT_EQ) will fire. Perhaps: int rv = .... rv = callback.GetResult(rv); EXPECT_EQ(ERR_NPN_NEGOTIATION_FAILED, rv);
bnc@chromium.org changed reviewers: + rch@chromium.org
Ryan: PTAL at patch sets 3 and 4, and decide where to fix this bug, and whether to call MaybeMarkAlternativeServiceBroken(). Thanks.
Calling MarkAlternateiveServiceBroken seems like it's definitely a good idea too. https://codereview.chromium.org/1121043002/diff/100001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1121043002/diff/100001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1110: if (IsAlternate()) nit: I think this should be IsSpdyAlternate(), since that's the only type of AlternateJob which does NPN at all. You could also do something like this up at the start of the method. if (IsAlternate()) DCHECK(IsSpdyAlternate()) to document that only SPDY alternate jobs hit this codepath (and that QUIC jobs do not).
Ryan: PTAL. I chose the check in DoInitConnectionComplete (as in Patch Set 3), because all the other MaybeMarkAlternativeServiceBroken() and job_status logic is there too, also because out of three "next_state_ = state_create_stream" assignments, two is immediately preceeded by "using_spdy_ = true", and the check is false in those cases. https://codereview.chromium.org/1121043002/diff/100001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1121043002/diff/100001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1110: if (IsAlternate()) On 2015/05/05 18:31:55, Ryan Hamilton wrote: > nit: I think this should be IsSpdyAlternate(), since that's the only type of > AlternateJob which does NPN at all. Done. BTW I moved the check from here to DoInitConnectionComplete. > You could also do something like this up at the start of the method. > > if (IsAlternate()) > DCHECK(IsSpdyAlternate()) > > to document that only SPDY alternate jobs hit this codepath (and that QUIC jobs > do not). Done.
Code looks good, just a couple tiny comments. I'm a bit less clear about the tests. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:12015: EXPECT_EQ(ERR_NPN_NEGOTIATION_FAILED, callback.GetResult(rv)); It's not obvious to me that this should be user visible. I guess there's no problem?, but it seems odd. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:12061: data_refused.set_connect_data(MockConnect(ASYNC, ERR_CONNECTION_REFUSED)); I don't understand why this test sets connection refused. Can you elaborate? https://codereview.chromium.org/1121043002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1121043002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1106: DCHECK(!IsAlternate() || IsSpdyAlternate()); nit: Personally, I find it hard to read dchecks with "complex" predicates. If I do something which triggers this code, I often have to stare at it a few times to figure out what the unexpected behavior is. I'd write it as: if (IsAlternate()) DCHECK(IsSpdyAlternate()); Which makes it really obvious. But this is me personal preference. Feel free to keep it if you find it more readable this way. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1116: if (!using_spdy_) { Can you add a DHECK(!IsSpdyAlternate()) here?
Patchset #8 (id:140001) has been deleted
Ryan: PTAL. Thank you. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:12015: EXPECT_EQ(ERR_NPN_NEGOTIATION_FAILED, callback.GetResult(rv)); On 2015/05/05 20:51:12, Ryan Hamilton wrote: > It's not obvious to me that this should be user visible. I guess there's no > problem?, but it seems odd. This test passes without landing the rest of this CL. For an alternate job, |want_spdy_over_npn| is set, it is plumbed through to the socket logic, and that fails after ALPN or NPN negotiation if not HTTP/2 (or SPDY) is negotiated. If it makes things clearer, I can land this test in a separate CL. If you prefer to change the behavior, that should also go in that separate CL. Let me know. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:12061: data_refused.set_connect_data(MockConnect(ASYNC, ERR_CONNECTION_REFUSED)); On 2015/05/05 20:51:12, Ryan Hamilton wrote: > I don't understand why this test sets connection refused. Can you elaborate? |request2| is to |origin|, which has an alternative. I have an HTTP/1.1 connection warmed up to the alternative, and I want to test that |request2| does not pool to that. In order to do this, I fail the connection to origin. If |request2| pooled to the HTTP/1.1 connection, it would still go through. I'm checking that it does not. Since the alternative job is expected to fail (because it should not pool to an HTTP/1.1 connection), I cannot leave the connection to the origin hanging, otherwise the test would just time out. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_stream_f... File net/http/http_stream_factory_impl_job.cc (right): https://codereview.chromium.org/1121043002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1106: DCHECK(!IsAlternate() || IsSpdyAlternate()); On 2015/05/05 20:51:12, Ryan Hamilton wrote: > nit: Personally, I find it hard to read dchecks with "complex" predicates. If I > do something which triggers this code, I often have to stare at it a few times > to figure out what the unexpected behavior is. I'd write it as: > > if (IsAlternate()) > DCHECK(IsSpdyAlternate()); > > Which makes it really obvious. > > But this is me personal preference. Feel free to keep it if you find it more > readable this way. Good point. Done. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_stream_f... net/http/http_stream_factory_impl_job.cc:1116: if (!using_spdy_) { On 2015/05/05 20:51:12, Ryan Hamilton wrote: > Can you add a DHECK(!IsSpdyAlternate()) here? Done.
lgtm, though a suggestion for an addition test (which you can do in a follow up, if you think it's a good idea. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:12015: EXPECT_EQ(ERR_NPN_NEGOTIATION_FAILED, callback.GetResult(rv)); On 2015/05/06 13:21:55, Bence wrote: > On 2015/05/05 20:51:12, Ryan Hamilton wrote: > > It's not obvious to me that this should be user visible. I guess there's no > > problem?, but it seems odd. > > This test passes without landing the rest of this CL. For an alternate job, > |want_spdy_over_npn| is set, it is plumbed through to the socket logic, and that > fails after ALPN or NPN negotiation if not HTTP/2 (or SPDY) is negotiated. > > If it makes things clearer, I can land this test in a separate CL. If you > prefer to change the behavior, that should also go in that separate CL. Let me > know. Landing this now is totally fine. I guess this all makes sense. I was just a bit surprised to see an alternative-job failure being user visible. But I suppose that only happens when the main job fails and it fails before the alt job? https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:12061: data_refused.set_connect_data(MockConnect(ASYNC, ERR_CONNECTION_REFUSED)); On 2015/05/06 13:21:55, Bence wrote: > On 2015/05/05 20:51:12, Ryan Hamilton wrote: > > I don't understand why this test sets connection refused. Can you elaborate? > > |request2| is to |origin|, which has an alternative. I have an HTTP/1.1 > connection warmed up to the alternative, and I want to test that |request2| does > not pool to that. In order to do this, I fail the connection to origin. If > |request2| pooled to the HTTP/1.1 connection, it would still go through. I'm > checking that it does not. > > Since the alternative job is expected to fail (because it should not pool to an > HTTP/1.1 connection), I cannot leave the connection to the origin hanging, > otherwise the test would just time out. Mind adding a comment to this (and the previous) test which explains this. What you say makes perfect sense. Should we also have tests in which the main request succeeds and which the alt job fails? (In which we make sure that we only attempt 1 alt job even if we make several requests, and that they all go to the origin)
Ryan: PTAL. Thanks. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:12015: EXPECT_EQ(ERR_NPN_NEGOTIATION_FAILED, callback.GetResult(rv)); On 2015/05/06 21:32:53, Ryan Hamilton wrote: > On 2015/05/06 13:21:55, Bence wrote: > > On 2015/05/05 20:51:12, Ryan Hamilton wrote: > > > It's not obvious to me that this should be user visible. I guess there's no > > > problem?, but it seems odd. > > > > This test passes without landing the rest of this CL. For an alternate job, > > |want_spdy_over_npn| is set, it is plumbed through to the socket logic, and > that > > fails after ALPN or NPN negotiation if not HTTP/2 (or SPDY) is negotiated. > > > > If it makes things clearer, I can land this test in a separate CL. If you > > prefer to change the behavior, that should also go in that separate CL. Let > me > > know. > > Landing this now is totally fine. I guess this all makes sense. I was just a bit > surprised to see an alternative-job failure being user visible. But I suppose > that only happens when the main job fails and it fails before the alt job? Yes, that's my understanding. https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:12061: data_refused.set_connect_data(MockConnect(ASYNC, ERR_CONNECTION_REFUSED)); On 2015/05/06 21:32:53, Ryan Hamilton wrote: > On 2015/05/06 13:21:55, Bence wrote: > > On 2015/05/05 20:51:12, Ryan Hamilton wrote: > > > I don't understand why this test sets connection refused. Can you elaborate? > > > > |request2| is to |origin|, which has an alternative. I have an HTTP/1.1 > > connection warmed up to the alternative, and I want to test that |request2| > does > > not pool to that. In order to do this, I fail the connection to origin. If > > |request2| pooled to the HTTP/1.1 connection, it would still go through. I'm > > checking that it does not. > > > > Since the alternative job is expected to fail (because it should not pool to > an > > HTTP/1.1 connection), I cannot leave the connection to the origin hanging, > > otherwise the test would just time out. > > Mind adding a comment to this (and the previous) test which explains this. What > you say makes perfect sense. Done. > Should we also have tests in which the main request succeeds and which the alt > job fails? (In which we make sure that we only attempt 1 alt job even if we make > several requests, and that they all go to the origin) I don't see why we would need them. In this CL, I am adding tests to enforce that an HTTP/2 alternative service does not go through HTTP/1.1. But the Job to the origin can use HTTP/1.1, so what exactly would we test? Also, why would we attempt only one alternate Job? Because the alternative service is marked as broken? There is currently no test for that, I can add one, but that should be a separate CL.
lgtm https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... File net/http/http_network_transaction_unittest.cc (right): https://codereview.chromium.org/1121043002/diff/120001/net/http/http_network_... net/http/http_network_transaction_unittest.cc:12061: data_refused.set_connect_data(MockConnect(ASYNC, ERR_CONNECTION_REFUSED)); On 2015/05/08 14:46:13, Bence wrote: > On 2015/05/06 21:32:53, Ryan Hamilton wrote: > > On 2015/05/06 13:21:55, Bence wrote: > > > On 2015/05/05 20:51:12, Ryan Hamilton wrote: > > > > I don't understand why this test sets connection refused. Can you > elaborate? > > > > > > |request2| is to |origin|, which has an alternative. I have an HTTP/1.1 > > > connection warmed up to the alternative, and I want to test that |request2| > > does > > > not pool to that. In order to do this, I fail the connection to origin. If > > > |request2| pooled to the HTTP/1.1 connection, it would still go through. > I'm > > > checking that it does not. > > > > > > Since the alternative job is expected to fail (because it should not pool to > > an > > > HTTP/1.1 connection), I cannot leave the connection to the origin hanging, > > > otherwise the test would just time out. > > > > Mind adding a comment to this (and the previous) test which explains this. > What > > you say makes perfect sense. > > Done. > > > Should we also have tests in which the main request succeeds and which the alt > > job fails? (In which we make sure that we only attempt 1 alt job even if we > make > > several requests, and that they all go to the origin) > > I don't see why we would need them. In this CL, I am adding tests to enforce > that an HTTP/2 alternative service does not go through HTTP/1.1. But the Job to > the origin can use HTTP/1.1, so what exactly would we test? This would be testing that in the "normal" case, the Alt-Svc job failure is not user visible. The current set of tests only shows Alt-Svc failures that are user visible. These new tests are great! But some additional tests would give broader coverage of the expected behaviors. > Also, why would we > attempt only one alternate Job? Because the alternative service is marked as > broken? There is currently no test for that, I can add one, but that should be > a separate CL. Separate CL is totally fine.
The CQ bit was checked by bnc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1121043002/#ps180001 (title: "Re: #13.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1121043002/180001
Message was sent while issue was closed.
Committed patchset #9 (id:180001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/5452e2a52b71e3a2a4c471c864155831a9a59951 Cr-Commit-Position: refs/heads/master@{#328964} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
