Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(426)

Issue 559373003: Landing Recent QUIC Changes. (Closed)

Created:
6 years, 3 months ago by ramant (doing other things)
Modified:
6 years, 3 months ago
Reviewers:
Ryan Hamilton
CC:
chromium-reviews, cbentzel+watch_chromium.org, Ryan Hamilton, Jana, alyssar, Ian Swett, rjshade
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Landing Recent QUIC Changes. Discard all useless QUIC retransmissions instead of only the most recent useless retransmission. May reduce the chances of truncated acks. Merge internal change: 75561951 https://codereview.chromium.org/564663005/ Add a separate QUIC_CONNECTION_OVERALL_TIMED_OUT to identify overall timeouts from idle timeouts and increase the initial overall timeout from 5s to 10s. Merge internal change: 75544619 https://codereview.chromium.org/572083003/ Fix QUIC crash on ConnectionClose, protected behind renamed flag FLAGS_close_quic_connection_unfinished_streams_2 (defaulted to false). With this change we avoid trying to close the connection while driving the QUIC internal server response pipeline, as the call to SendConnectionClose now happens during PostProcessAfterData, which is only ever called after receiving new data. With a hacky client/QUIC internal server combo (sending > 100 requests, but never sending FIN/RST) I've verified that with this change we don't hit the DFATAL from the attached bug. Still not entirely sure why Chrome would trigger this, b/17402744 is open to investigate why it ever opens more than kMaxStreams. Merge internal change: 75541290 https://codereview.chromium.org/575593002/ Add a LOG(DFATAL) to a case where packets are acked before being sent. Merge internal change: 75537267 https://codereview.chromium.org/575573003/ Allow number of open QUIC streams to grow 10% beyond configured limit at the QUIC internal server before closing connection. In the following situation Chrome may open more streams than the configured limit: * opens kMaxStreams streams * sends a FIN for one of the streams, now has kMaxStreams-1 streams open * opens a new stream * FIN gets lost on way to QUIC internal server * QUIC internal server terminates connection when new stream packet arrives Rather than making Chrome keep track of when the server has acknowledged a FIN, just give 10% breathing room which should avoid this case. Merge internal change: 75393293 https://codereview.chromium.org/557363003/ client-only - Change QUIC clients to use the initial RTT estimate that they provide to the server instead of the default. Re-enabled LimitCongestionWindowAndRTT end_to_end unittest (since we have fixed lot of flaky tests). Will test chromeos valgrind. Merge internal change: 75389539 https://codereview.chromium.org/573763003/ BUG=321870 Increase flow control receive window of quic_client_bin.cc, by adding a flag that defaults to 10 Mb. Added --flow-control-window-bytes as argument. Merge internal change: 75381087 https://codereview.chromium.org/569353003/ Call QuicSentPacketManager's OnPacketSerialized or OnRetransmittedPacket right before OnPacketSent, in preparation for combining the three methods into one. Merge internal change: 75340764 https://codereview.chromium.org/566743003/ Instead of initializing QuicSession::next_stream_id_ to 3 in the construct, and then setting it to 5 in InitializeSession simply initialize it to 5 in the constructor. Merge internal change: 75335114 https://codereview.chromium.org/562793003/ Do not allow a QUIC connection to timeout if no packets have been sent or received. Protected by FLAGS_quic_timeouts_require_activity. This seems to happen on windows from time to time and causes problems there. Merge internal change: 75304108 https://codereview.chromium.org/571993002/ Minor change to QUIC's retransmission to not link old retransmissions with new ones when the version or encryption changes. Allows the least_unacked to be raised more quickly, preventing spurious acks. Merge internal change: 75290729 https://codereview.chromium.org/572703003/ Removed unnecessary static_cast and replaced it with "lu". Minor change to keep the code similar to internal code. Merge internal change: 75263809 https://codereview.chromium.org/566373003/ QUIC cleanup to remove an unused bool argument in SendOrQueuePacket and OnSerializedPacket. Merge internal change: 75208575 https://codereview.chromium.org/553583007/ R=rch@chromium.org Committed: https://crrev.com/31e9fd65aa1f2d7753f356fc267743a039fbd980 Cr-Commit-Position: refs/heads/master@{#295014}

Patch Set 1 #

Patch Set 2 : Fix compiler errors #

Unified diffs Side-by-side diffs Delta from patch set Stats (+607 lines, -288 lines) Patch
M net/quic/quic_config.h View 1 chunk +4 lines, -0 lines 0 comments Download
M net/quic/quic_config.cc View 2 chunks +11 lines, -3 lines 0 comments Download
M net/quic/quic_config_test.cc View 5 chunks +8 lines, -8 lines 0 comments Download
M net/quic/quic_connection.h View 5 chunks +37 lines, -34 lines 0 comments Download
M net/quic/quic_connection.cc View 18 chunks +95 lines, -73 lines 0 comments Download
M net/quic/quic_connection_test.cc View 3 chunks +49 lines, -2 lines 0 comments Download
M net/quic/quic_crypto_client_stream_test.cc View 1 chunk +1 line, -1 line 0 comments Download
M net/quic/quic_flags.h View 1 chunk +3 lines, -1 line 0 comments Download
M net/quic/quic_flags.cc View 1 chunk +9 lines, -1 line 0 comments Download
M net/quic/quic_packet_generator.h View 1 chunk +1 line, -1 line 0 comments Download
M net/quic/quic_packet_generator_test.cc View 24 chunks +44 lines, -53 lines 0 comments Download
M net/quic/quic_protocol.h View 1 6 chunks +16 lines, -11 lines 0 comments Download
M net/quic/quic_protocol.cc View 3 chunks +6 lines, -3 lines 0 comments Download
M net/quic/quic_sent_packet_manager.h View 1 chunk +1 line, -1 line 0 comments Download
M net/quic/quic_sent_packet_manager.cc View 3 chunks +12 lines, -7 lines 0 comments Download
M net/quic/quic_sent_packet_manager_test.cc View 7 chunks +39 lines, -16 lines 0 comments Download
M net/quic/quic_server_session.cc View 1 chunk +2 lines, -1 line 0 comments Download
M net/quic/quic_session.h View 2 chunks +3 lines, -3 lines 0 comments Download
M net/quic/quic_session.cc View 8 chunks +26 lines, -15 lines 0 comments Download
M net/quic/quic_session_test.cc View 3 chunks +6 lines, -2 lines 0 comments Download
M net/quic/quic_unacked_packet_map.h View 1 chunk +3 lines, -0 lines 0 comments Download
M net/quic/quic_unacked_packet_map.cc View 7 chunks +53 lines, -16 lines 0 comments Download
M net/quic/quic_unacked_packet_map_test.cc View 1 3 chunks +81 lines, -2 lines 0 comments Download
M net/quic/quic_utils.cc View 2 chunks +2 lines, -0 lines 0 comments Download
M net/quic/test_tools/mock_crypto_client_stream.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M net/quic/test_tools/quic_test_utils.h View 1 chunk +1 line, -3 lines 0 comments Download
M net/quic/test_tools/quic_test_utils.cc View 1 chunk +8 lines, -7 lines 0 comments Download
M net/tools/quic/end_to_end_test.cc View 2 chunks +5 lines, -7 lines 0 comments Download
M net/tools/quic/quic_client_bin.cc View 4 chunks +22 lines, -1 line 0 comments Download
M net/tools/quic/quic_server_session.cc View 1 chunk +2 lines, -1 line 0 comments Download
M net/tools/quic/quic_server_session_test.cc View 2 chunks +55 lines, -13 lines 0 comments Download

Messages

Total messages: 6 (1 generated)
ramant (doing other things)
6 years, 3 months ago (2014-09-15 23:33:27 UTC) #1
Ryan Hamilton
lgtm
6 years, 3 months ago (2014-09-16 03:15:52 UTC) #2
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/559373003/20001
6 years, 3 months ago (2014-09-16 04:38:49 UTC) #4
commit-bot: I haz the power
Committed patchset #2 (id:20001) as 7e070a326e26b1aeb233fc48c7e7cb4ae11d2438
6 years, 3 months ago (2014-09-16 05:22:58 UTC) #5
commit-bot: I haz the power
6 years, 3 months ago (2014-09-16 05:24:29 UTC) #6
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/31e9fd65aa1f2d7753f356fc267743a039fbd980
Cr-Commit-Position: refs/heads/master@{#295014}

Powered by Google App Engine
This is Rietveld 408576698