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

Issue 1439653002: Not sending QUIC packets if the send alarm is set. Behind FLAGS_respect_send_alarm (Closed)

Created:
5 years, 1 month ago by rjshade
Modified:
5 years, 1 month ago
Reviewers:
Ryan Hamilton
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@107277639
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Not sending QUIC packets if the send alarm is set. Behind FLAGS_respect_send_alarm This fixes a subtle bug where we weren't always respecting priorities when writing. I posit that if we are send limited by the sent packet manager, we expect to resume with OnCanWrite, and resume the highest priority session. Unfortunately right now that's not the case. When the highest priority session gets blocked by the sent packet manager it schedules the alarm. However it's perfectly possible that before the alarm fires, we get an incoming request, process it, that stream checks to see if it can write, and when CanWrite checks now(), we are past the time when the alarm should have fired (due to doing a lot of work, busy machine, what have you) and write data for the random stream. Merge internal change: 107380116 R=rch@chromium.org BUG=

Patch Set 1 #

Total comments: 1

Patch Set 2 : Disable flag #

Unified diffs Side-by-side diffs Delta from patch set Stats (+13 lines, -1 line) Patch
M net/quic/quic_connection.cc View 2 chunks +8 lines, -1 line 0 comments Download
M net/quic/quic_flags.h View 1 chunk +1 line, -0 lines 0 comments Download
M net/quic/quic_flags.cc View 1 1 chunk +4 lines, -0 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 4 (0 generated)
rjshade
5 years, 1 month ago (2015-11-11 16:51:08 UTC) #1
Ryan Hamilton
https://codereview.chromium.org/1439653002/diff/1/net/quic/quic_flags.cc File net/quic/quic_flags.cc (right): https://codereview.chromium.org/1439653002/diff/1/net/quic/quic_flags.cc#newcode124 net/quic/quic_flags.cc:124: bool FLAGS_respect_send_alarm = true; Ian mentioned this morning that ...
5 years, 1 month ago (2015-11-11 17:56:23 UTC) #2
rjshade
On 2015/11/11 17:56:23, Ryan Hamilton wrote: > https://codereview.chromium.org/1439653002/diff/1/net/quic/quic_flags.cc > File net/quic/quic_flags.cc (right): > > https://codereview.chromium.org/1439653002/diff/1/net/quic/quic_flags.cc#newcode124 ...
5 years, 1 month ago (2015-11-11 17:57:37 UTC) #3
Ryan Hamilton
5 years, 1 month ago (2015-11-11 22:13:52 UTC) #4
lgtm

Powered by Google App Engine
This is Rietveld 408576698