Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1208933004:
QUIC - disable QUIC under recent pathological connection errors. (Closed)
DescriptionQUIC - disable QUIC under recent pathological connection errors.
The problematic cases are:
1) timeouts with streams open.
2) public reset post crypto handshake.
In each case, a K of N thresholding is applied, for example disable
QUIC if two of recent twenty connections experienced the problem.
Goal - avoid QUIC for users on networks that have pathological impediments.
+ Initially, this is configured in a measurment only mode, to
determine what threshold values seem most promising. The twenty most
recent connections will be considered, and tracked via UMA histograms.
BUG=505891
Committed: https://crrev.com/1e53b646828a7ec8b4e1465b0f203cf89943ac16
Cr-Commit-Position: refs/heads/master@{#337926}
Patch Set 1 #
Total comments: 50
Patch Set 2 : Revised after first round of review feedback (no net-internals yet). #Patch Set 3 : net-internals additions #
Total comments: 12
Patch Set 4 : Revised after second round of review feedback. #Patch Set 5 : Move dead code out of OnHandshakeConfirmed to where it will actually work (spotted by Ryan). #
Total comments: 26
Patch Set 6 : Revised after third round of review feedback. #
Total comments: 6
Patch Set 7 : Revised after fourth round of review feedback. #
Messages
Total messages: 27 (8 generated)
ckrasic@chromium.org changed reviewers: + askvitkine@chromium.org, rch@google.com
rch@chromium.org changed reviewers: + rch@chromium.org - rch@google.com
SWEET! This is great. I can't wait to see what we learn! Oh! Now that I think about it, we need to update net-internals to include information about QUIC being disabled. I can walk you through that code when you have some time. https://codereview.chromium.org/1208933004/diff/1/net/http/http_network_sessi... File net/http/http_network_session.h (right): https://codereview.chromium.org/1208933004/diff/1/net/http/http_network_sessi... net/http/http_network_session.h:131: int quic_threshold_timeouts_streams_open; I assume you're planning to wire these up to finch in a different CL? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... File net/quic/quic_client_session.cc (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.cc:731: } Alternatively, you could do: } else if (error == QUIC_PUBLIC_RESET) { epitaph_ = QUIC_EPITAPH_PUBLIC_RESET_POST_HANDSHAKE; } which would avoid the need to introduce the crypto_handshake_confirmed local variable. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_session.h File net/quic/quic_client_session.h (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:44: // Epitaphs will be used to track the most recent departed sessions, Can you define "epitath" in this context? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:49: enum QuicClientSessionEpitaph { nit: might as well nest this enum inside of QuicClientSession as QuicClientSession::Epitath. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:50: QUIC_EPITAPH_RIP = 0, What does "RIP" mean in this context? Perhaps add a comment? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:53: QUIC_EPITAPH_MAX = 3, Could the packet loss code take advantage of this, or does that live up in the Factory, not down here in the Session? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:634: // TODO: need a separate max to track this. TODO:(ckrasic) since you have the context? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:644: } I think this is too late for histograms to be of much use. By the time the network stack is being shut down, the histograms services is already dead. This won't crash, but I don't think it'll end up logging anything :/ Perhaps log when the total number of connections is > some value? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:864: return (QuicDisabledReason(port) != QUIC_NO_ERROR); nit: no need for the extra ()s https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:905: return false; Your call, but I might write this as: if (error_code == NO_ERROR) return true; // Close QUIC ... .. (Just to remove a level of nesting now that we're using different explicit return statements. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:947: if ((int)epitaphs_.size() == max_epitaphs_) { nit: static_cast https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:966: public_resets_post_handshake_, max_public_resets_post_handshake_); I'm not following the logic of resetting max_ here. Can you add a comment? (I'm sure it makes sense) https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factory.h File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:160: QuicErrorCode QuicDisabledReason(uint16 port); Instead of QuicErrorCodes (which intended as on-the-wire error codes to communicate problems with a peer) I recommend using a new enum with just the few reasons. Something like: QuicStreamFactory::DisabledReason https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:255: typedef std::deque<QuicClientSessionEpitaph> EpitaphsQueue; nit: remove the newline before to be consistent with other typedefs. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:384: // Keep track of stats for recently departed connections, using a nit: instead of "departed", how about "closed" which matches usage elsewhere. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:388: // Events that can trigger disaabling QUIC nit: disaabling -> disabling https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:389: int public_resets_post_handshake_; nit: num_public_resets_post_handshake_ and num_timeouts_with_open_streams_? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... File net/quic/quic_stream_factory_test.cc (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:163: return factory->timeouts_streams_open_; here and elsewhere, I think "timeouts with open streams" is easier to read than "timeouts streams open" (which is admittedly shorter :>) https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1805: DVLOG(1) << "Create 1st session and trigger pulic reset post handshake"; The session has already been created here, right? Perhaps "Created 1st session. Now trigger ..."? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1810: run_loop.RunUntilIdle(); Does it work if you do base::RunLoop().RunUntilIdle()? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1816: // Test N-in-a-row public reset post handshakes.. Does "N-in-a-row" simply mean 2, or is there another test which does Good, Bad, Good and verifies that QUIC is disabled there too? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1833: QuicStreamFactoryPeer::IsQuicDisabled(&factory_, host_port_pair_.port())); Can you also test your disabled reason method here too? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1901: // Test N-in-a-row public reset post handshakes.. s/public reset post handshakes/timeouts with open streams/?
I've addressed the first round of feedback, except for the net-internals. https://codereview.chromium.org/1208933004/diff/1/net/http/http_network_sessi... File net/http/http_network_session.h (right): https://codereview.chromium.org/1208933004/diff/1/net/http/http_network_sessi... net/http/http_network_session.h:131: int quic_threshold_timeouts_streams_open; On 2015/06/30 18:55:31, Ryan Hamilton wrote: > I assume you're planning to wire these up to finch in a different CL? Yes. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... File net/quic/quic_client_session.cc (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.cc:731: } On 2015/06/30 18:55:31, Ryan Hamilton wrote: > Alternatively, you could do: > > } else if (error == QUIC_PUBLIC_RESET) { > epitaph_ = QUIC_EPITAPH_PUBLIC_RESET_POST_HANDSHAKE; > } > > which would avoid the need to introduce the crypto_handshake_confirmed local > variable. Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_session.h File net/quic/quic_client_session.h (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:44: // Epitaphs will be used to track the most recent departed sessions, On 2015/06/30 18:55:31, Ryan Hamilton wrote: > Can you define "epitath" in this context? I've dropped epitaph everywhere, and switched to DisabledReason. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:49: enum QuicClientSessionEpitaph { On 2015/06/30 18:55:31, Ryan Hamilton wrote: > nit: might as well nest this enum inside of QuicClientSession as > QuicClientSession::Epitath. I've moved it (enum QuicDisabledReason) to quic_types.h, as it's referenced from both QuicClientSession and QuicStreamFactory. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:50: QUIC_EPITAPH_RIP = 0, On 2015/06/30 18:55:31, Ryan Hamilton wrote: > What does "RIP" mean in this context? Perhaps add a comment? Corresponding entry in QuicDisabledReason has a comment now. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:53: QUIC_EPITAPH_MAX = 3, On 2015/06/30 18:55:31, Ryan Hamilton wrote: > Could the packet loss code take advantage of this, or does that live up in the > Factory, not down here in the Session? Somewhat. As per above, I've moved it and also added a QUIC_DISABLED_BAD_PACKET_LOSS to the revised enum. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:634: // TODO: need a separate max to track this. On 2015/06/30 18:55:32, Ryan Hamilton wrote: > TODO:(ckrasic) since you have the context? Removed, the comment was obsolete. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:644: } On 2015/06/30 18:55:32, Ryan Hamilton wrote: > I think this is too late for histograms to be of much use. By the time the > network stack is being shut down, the histograms services is already dead. This > won't crash, but I don't think it'll end up logging anything :/ Perhaps log > when the total number of connections is > some value? Ok. I've moved these to where max_* get incremented. So the histograms will be cumulative now. I think it will still give me what I need which is data guide setting the thresholds. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:864: return (QuicDisabledReason(port) != QUIC_NO_ERROR); On 2015/06/30 18:55:32, Ryan Hamilton wrote: > nit: no need for the extra ()s Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:905: return false; On 2015/06/30 18:55:32, Ryan Hamilton wrote: > Your call, but I might write this as: > > if (error_code == NO_ERROR) > return true; > > // Close QUIC ... > .. > > (Just to remove a level of nesting now that we're using different explicit > return statements. Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:947: if ((int)epitaphs_.size() == max_epitaphs_) { On 2015/06/30 18:55:32, Ryan Hamilton wrote: > nit: static_cast Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:966: public_resets_post_handshake_, max_public_resets_post_handshake_); On 2015/06/30 18:55:31, Ryan Hamilton wrote: > I'm not following the logic of resetting max_ here. Can you add a comment? (I'm > sure it makes sense) I changed this so it is hopefully more direct and understandable. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factory.h File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:160: QuicErrorCode QuicDisabledReason(uint16 port); On 2015/06/30 18:55:32, Ryan Hamilton wrote: > Instead of QuicErrorCodes (which intended as on-the-wire error codes to > communicate problems with a peer) I recommend using a new enum with just the few > reasons. Something like: QuicStreamFactory::DisabledReason I went with QuicDisabledReason in quic_types.h https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:255: typedef std::deque<QuicClientSessionEpitaph> EpitaphsQueue; On 2015/06/30 18:55:32, Ryan Hamilton wrote: > nit: remove the newline before to be consistent with other typedefs. Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:384: // Keep track of stats for recently departed connections, using a On 2015/06/30 18:55:32, Ryan Hamilton wrote: > nit: instead of "departed", how about "closed" which matches usage elsewhere. Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:388: // Events that can trigger disaabling QUIC On 2015/06/30 18:55:32, Ryan Hamilton wrote: > nit: disaabling -> disabling Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.h:389: int public_resets_post_handshake_; On 2015/06/30 18:55:32, Ryan Hamilton wrote: > nit: num_public_resets_post_handshake_ and num_timeouts_with_open_streams_? Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... File net/quic/quic_stream_factory_test.cc (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:163: return factory->timeouts_streams_open_; On 2015/06/30 18:55:32, Ryan Hamilton wrote: > here and elsewhere, I think "timeouts with open streams" is easier to read than > "timeouts streams open" (which is admittedly shorter :>) Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1805: DVLOG(1) << "Create 1st session and trigger pulic reset post handshake"; On 2015/06/30 18:55:32, Ryan Hamilton wrote: > The session has already been created here, right? Perhaps "Created 1st session. > Now trigger ..."? Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1810: run_loop.RunUntilIdle(); On 2015/06/30 18:55:32, Ryan Hamilton wrote: > Does it work if you do base::RunLoop().RunUntilIdle()? Nope. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1816: // Test N-in-a-row public reset post handshakes.. On 2015/06/30 18:55:32, Ryan Hamilton wrote: > Does "N-in-a-row" simply mean 2, or is there another test which does Good, Bad, > Good and verifies that QUIC is disabled there too? It did mean two. I've revised the comment. I've also added more tests, specifically cased for two of three (bad, good, bad), and two of four (bad, good, good, bad). https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1833: QuicStreamFactoryPeer::IsQuicDisabled(&factory_, host_port_pair_.port())); On 2015/06/30 18:55:32, Ryan Hamilton wrote: > Can you also test your disabled reason method here too? Done. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1901: // Test N-in-a-row public reset post handshakes.. On 2015/06/30 18:55:32, Ryan Hamilton wrote: > s/public reset post handshakes/timeouts with open streams/? Done.
Looking good. A few small comments. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_session.h File net/quic/quic_client_session.h (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:49: enum QuicClientSessionEpitaph { On 2015/07/01 19:06:19, Buck wrote: > On 2015/06/30 18:55:31, Ryan Hamilton wrote: > > nit: might as well nest this enum inside of QuicClientSession as > > QuicClientSession::Epitath. > > I've moved it (enum QuicDisabledReason) to quic_types.h, as it's referenced from > both QuicClientSession and QuicStreamFactory. quic_types.h is part of the shared code. Since these enums are chromium-specific, we should probably keep them in chromium-specific code. I recommend QuicClientSession::DisabledReason or perhaps QuicStreamFactory::DisabledReason? https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory.cc:966: public_resets_post_handshake_, max_public_resets_post_handshake_); On 2015/07/01 19:06:19, Buck wrote: > On 2015/06/30 18:55:31, Ryan Hamilton wrote: > > I'm not following the logic of resetting max_ here. Can you add a comment? > (I'm > > sure it makes sense) > > I changed this so it is hopefully more direct and understandable. Oh, I see. I was confusing max_disabled_reasons_ with max_<particular_reason>_. *facepalm* https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... File net/quic/quic_stream_factory_test.cc (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_stream_factor... net/quic/quic_stream_factory_test.cc:1810: run_loop.RunUntilIdle(); On 2015/07/01 19:06:20, Buck wrote: > On 2015/06/30 18:55:32, Ryan Hamilton wrote: > > Does it work if you do base::RunLoop().RunUntilIdle()? > > Nope. *head explodes* https://codereview.chromium.org/1208933004/diff/40001/chrome/browser/resource... File chrome/browser/resources/net_internals/quic_view.html (right): https://codereview.chromium.org/1208933004/diff/40001/chrome/browser/resource... chrome/browser/resources/net_internals/quic_view.html:13: <li>QUIC dynamically disabled: <span jscontent="disabled_reason"></span></li> You could consider making this whole <li> conditional on disabled_reason being non-empty, but your call. https://codereview.chromium.org/1208933004/diff/40001/net/http/http_network_s... File net/http/http_network_session.cc (right): https://codereview.chromium.org/1208933004/diff/40001/net/http/http_network_s... net/http/http_network_session.cc:66: const int32 kQuicMaxEpitaphs = 20; ditto https://codereview.chromium.org/1208933004/diff/40001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/1208933004/diff/40001/net/http/http_network_s... net/http/http_network_session.h:129: int quic_max_epitaphs; Were you planning to remove "epitaphs" here too? https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_protocol.h File net/quic/quic_protocol.h (right): https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_protocol.... net/quic/quic_protocol.h:548: QUIC_TIMEOUTS_WITH_OPEN_STREAMS = 73, Since you're using the QuicDisabledReason enum, are these still needed? https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:871: } nit: It looks like this is just duplicating the logic in the previous method, but returning strings instead of enums. So how about: QuicDisabledReason reason = QuicDisabledReason(443); switch (reason) { case QUIC_DISABLED_BAD_PACKET_LOSS_RATE: return "bad packet loss rate" ... } https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.h:162: std::string QuicDisabledReasonString() const; nit: const std::string& ? (Or if that doesn't work because it'd be returning a ref to a temporary, how bout a const char*? I'm not entirely sure where this is used. Perhaps it's not worth saving a copy...
Issues addressed. I think I did the correct thing in quic_view.html, but if you could check it would be great. https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_session.h File net/quic/quic_client_session.h (right): https://codereview.chromium.org/1208933004/diff/1/net/quic/quic_client_sessio... net/quic/quic_client_session.h:49: enum QuicClientSessionEpitaph { On 2015/07/06 18:25:41, Ryan Hamilton wrote: > On 2015/07/01 19:06:19, Buck wrote: > > On 2015/06/30 18:55:31, Ryan Hamilton wrote: > > > nit: might as well nest this enum inside of QuicClientSession as > > > QuicClientSession::Epitath. > > > > I've moved it (enum QuicDisabledReason) to quic_types.h, as it's referenced > from > > both QuicClientSession and QuicStreamFactory. > > quic_types.h is part of the shared code. Since these enums are > chromium-specific, we should probably keep them in chromium-specific code. I > recommend QuicClientSession::DisabledReason or perhaps > QuicStreamFactory::DisabledReason? Ok, QuicClientSession::DisabledReason it is. https://codereview.chromium.org/1208933004/diff/40001/chrome/browser/resource... File chrome/browser/resources/net_internals/quic_view.html (right): https://codereview.chromium.org/1208933004/diff/40001/chrome/browser/resource... chrome/browser/resources/net_internals/quic_view.html:13: <li>QUIC dynamically disabled: <span jscontent="disabled_reason"></span></li> On 2015/07/06 18:25:41, Ryan Hamilton wrote: > You could consider making this whole <li> conditional on disabled_reason being > non-empty, but your call. Done. https://codereview.chromium.org/1208933004/diff/40001/net/http/http_network_s... File net/http/http_network_session.cc (right): https://codereview.chromium.org/1208933004/diff/40001/net/http/http_network_s... net/http/http_network_session.cc:66: const int32 kQuicMaxEpitaphs = 20; On 2015/07/06 18:25:41, Ryan Hamilton wrote: > ditto Done. https://codereview.chromium.org/1208933004/diff/40001/net/http/http_network_s... File net/http/http_network_session.h (right): https://codereview.chromium.org/1208933004/diff/40001/net/http/http_network_s... net/http/http_network_session.h:129: int quic_max_epitaphs; On 2015/07/06 18:25:41, Ryan Hamilton wrote: > Were you planning to remove "epitaphs" here too? Yes. Thanks for catching it. https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_protocol.h File net/quic/quic_protocol.h (right): https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_protocol.... net/quic/quic_protocol.h:548: QUIC_TIMEOUTS_WITH_OPEN_STREAMS = 73, On 2015/07/06 18:25:41, Ryan Hamilton wrote: > Since you're using the QuicDisabledReason enum, are these still needed? I think so, yes. These are used in QuicStreamFactory::OnHandshakeConfirmed() in calls to CloseSessionOnError*. https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:871: } On 2015/07/06 18:25:41, Ryan Hamilton wrote: > nit: It looks like this is just duplicating the logic in the previous method, > but returning strings instead of enums. So how about: > > QuicDisabledReason reason = QuicDisabledReason(443); > switch (reason) { > case QUIC_DISABLED_BAD_PACKET_LOSS_RATE: > return "bad packet loss rate" > ... > } Done. https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/1208933004/diff/40001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.h:162: std::string QuicDisabledReasonString() const; On 2015/07/06 18:25:41, Ryan Hamilton wrote: > nit: const std::string& ? (Or if that doesn't work because it'd be returning a > ref to a temporary, how bout a const char*? I'm not entirely sure where this is > used. Perhaps it's not worth saving a copy... going with const char* fyi, it is used in HttpNetworkSession::QuicInfoToValue().
FYI: using some one-off code changes in my local build, I verified that the optional list item in quic_view.html works as intended.
lgtm https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_client_se... File net/quic/quic_client_session.h (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_client_se... net/quic/quic_client_session.h:47: // Reasons we may disable QUIC, that is under certain pathological nit: Please don't use the first person in comments. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_protocol.h File net/quic/quic_protocol.h (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_protocol.... net/quic/quic_protocol.h:548: QUIC_TIMEOUTS_WITH_OPEN_STREAMS = 74, FYI: You'll need to merge this part of your CL to the internal repository. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:870: return "bad packet loss rate"; nit: Can you use a leading Capital and end with a period. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.h:162: // Returns reason QUIC is disabled as string for net-internals. Can you comment on what is returned if QUIC is not disabled?
asvitkine@chromium.org changed reviewers: + asvitkine@chromium.org - askvitkine@chromium.org
https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_client_se... File net/quic/quic_client_session.h (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_client_se... net/quic/quic_client_session.h:209: const QuicDisabledReason disabled_reason() const { return disabled_reason_; } Nit: No need for leading const. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:963: enum QuicClientSession::QuicDisabledReason disabled_reason; Nit: No need for "enum". https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:969: num_public_resets_post_handshake_--; Nit: I think --foo; and ++foo; are preferred in Chromium code. Please fix throughout. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22550: + connections experience: public reset post crypto handshake, or timeouts Nit: Extra space. You also have some extra spaces in other descriptions. Please fix. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22551: + with streams open. QUIC is disabled until the next reboot of Chrome. Please mention when this is logged. Same for the other ones. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22555: +<histogram name="Net.QuicStreamFactory.PublicResetsPostHandshake"> Add a units="" attr. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22556: + \ Remove \ https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22575: +<histogram name="Net.QuicStreamFactory.TimeoutsWithOpenStreams"> Add a units="" attr. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22576: + \ Remove \
Issues addressed, PTAL. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_client_se... File net/quic/quic_client_session.h (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_client_se... net/quic/quic_client_session.h:47: // Reasons we may disable QUIC, that is under certain pathological On 2015/07/07 03:09:37, Ryan Hamilton wrote: > nit: Please don't use the first person in comments. Done. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_client_se... net/quic/quic_client_session.h:209: const QuicDisabledReason disabled_reason() const { return disabled_reason_; } On 2015/07/07 15:15:29, Alexei Svitkine wrote: > Nit: No need for leading const. Done. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_protocol.h File net/quic/quic_protocol.h (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_protocol.... net/quic/quic_protocol.h:548: QUIC_TIMEOUTS_WITH_OPEN_STREAMS = 74, On 2015/07/07 03:09:37, Ryan Hamilton wrote: > FYI: You'll need to merge this part of your CL to the internal repository. Acknowledged. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.cc (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:870: return "bad packet loss rate"; On 2015/07/07 03:09:37, Ryan Hamilton wrote: > nit: Can you use a leading Capital and end with a period. Done. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:963: enum QuicClientSession::QuicDisabledReason disabled_reason; On 2015/07/07 15:15:29, Alexei Svitkine wrote: > Nit: No need for "enum". Done. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.cc:969: num_public_resets_post_handshake_--; On 2015/07/07 15:15:29, Alexei Svitkine wrote: > Nit: I think --foo; and ++foo; are preferred in Chromium code. Please fix > throughout. Done. https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... File net/quic/quic_stream_factory.h (right): https://codereview.chromium.org/1208933004/diff/80001/net/quic/quic_stream_fa... net/quic/quic_stream_factory.h:162: // Returns reason QUIC is disabled as string for net-internals. On 2015/07/07 03:09:37, Ryan Hamilton wrote: > Can you comment on what is returned if QUIC is not disabled? Done. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22550: + connections experience: public reset post crypto handshake, or timeouts On 2015/07/07 15:15:29, Alexei Svitkine wrote: > Nit: Extra space. You also have some extra spaces in other descriptions. Please > fix. Done. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22551: + with streams open. QUIC is disabled until the next reboot of Chrome. On 2015/07/07 15:15:29, Alexei Svitkine wrote: > Please mention when this is logged. Same for the other ones. Done. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22555: +<histogram name="Net.QuicStreamFactory.PublicResetsPostHandshake"> On 2015/07/07 15:15:29, Alexei Svitkine wrote: > Add a units="" attr. Done. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22556: + \ On 2015/07/07 15:15:29, Alexei Svitkine wrote: > Remove \ Done. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22575: +<histogram name="Net.QuicStreamFactory.TimeoutsWithOpenStreams"> On 2015/07/07 15:15:29, Alexei Svitkine wrote: > Add a units="" attr. Done. https://codereview.chromium.org/1208933004/diff/80001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:22576: + \ On 2015/07/07 15:15:29, Alexei Svitkine wrote: > Remove \ Done.
lgtm % remaining comments https://codereview.chromium.org/1208933004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1208933004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:22621: +<histogram name="Net.QuicStreamFactory.PublicResetsPostHandshake" units="count"> Nit: Wouldn't units="resets" make more sense? https://codereview.chromium.org/1208933004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:22640: +<histogram name="Net.QuicStreamFactory.TimeoutsWithOpenStreams" units="count"> units="timeouts"? https://codereview.chromium.org/1208933004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:22643: + Captures the maximum number of connection timeouts with streams open that Nit: You still have randomly doubled spaces in this description.
https://codereview.chromium.org/1208933004/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1208933004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:22621: +<histogram name="Net.QuicStreamFactory.PublicResetsPostHandshake" units="count"> On 2015/07/07 18:44:09, Alexei Svitkine wrote: > Nit: Wouldn't units="resets" make more sense? Done. https://codereview.chromium.org/1208933004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:22640: +<histogram name="Net.QuicStreamFactory.TimeoutsWithOpenStreams" units="count"> On 2015/07/07 18:44:09, Alexei Svitkine wrote: > units="timeouts"? Done. https://codereview.chromium.org/1208933004/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:22643: + Captures the maximum number of connection timeouts with streams open that On 2015/07/07 18:44:09, Alexei Svitkine wrote: > Nit: You still have randomly doubled spaces in this description. yikes, sorry I missed it! Done.
rch@, can you land this patch for me?
The CQ bit was checked by rch@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rch@chromium.org, asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1208933004/#ps120001 (title: "Revised after fourth round of review feedback.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208933004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
ckrasic@chromium.org changed reviewers: + eroman@chromium.org
commit-bot rejected because I was missing an OWNER for quic_view.html. ermoman@ can you take a look?
quick_view.html lgtm
The CQ bit was checked by ckrasic@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1208933004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/1e53b646828a7ec8b4e1465b0f203cf89943ac16 Cr-Commit-Position: refs/heads/master@{#337926} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
