Correctly handle SPDY GOAWAY frames.

Correctly handle SPDY GOAWAY frames.

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

[willchan no longer on Chromium, 2013-04-17 20:12:48]

I'm going to let akalin handle this instead :) Let me know if you'd prefer me
for any reason. I'll skim though.

[Ryan Hamilton, 2013-04-17 20:49:14]

On Wednesday, April 17, 2013, wrote:

> I'm going to let akalin handle this instead :) Let me know if you'd prefer
> me
> for any reason. I'll skim though.
>
>
https://codereview.chromium.**org/14232014/<https://codereview.chromium.org/1...
>

Sounds good. I sent it to you because you were in the office when I
finished. :)

[akalin, 2013-04-17 21:14:20]

looks mostly good, a few comments

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_session.cc#newc...
net/spdy/spdy_session.cc:1146: ActiveStreamMap new_active_streams;
Curious -- are we worried about re-entrancy here? If not, seems easier to use
lower_bound to find the first stream > last_good_stream_id and start deleting
from there

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_session.h#newco...
net/spdy/spdy_session.h:613: void CloseAllStreamsAfter(SpdyStreamId
last_good_stream_id,
add a comment for CloseAllStreamsAfter? Perhaps document the additional stuff
CloseAllStreams does over CloseAllStreamsAfter(0, _)

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2.cc
File net/spdy/spdy_test_util_spdy2.cc (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2...
net/spdy/spdy_test_util_spdy2.cc:152: // Construct a SPDY GOAWAY frame.
i don't know if it's worth having the same comment in both the .h and the .cc
file. But I guess it's consistent.

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2.h
File net/spdy/spdy_test_util_spdy2.h (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2...
net/spdy/spdy_test_util_spdy2.h:119: // Construct a SPDY GOAWAY frame.
document difference between this overload and the previous one?

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy3.cc
File net/spdy/spdy_test_util_spdy3.cc (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy3...
net/spdy/spdy_test_util_spdy3.cc:206: // Construct a SPDY GOAWAY frame.
here too

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy3.h
File net/spdy/spdy_test_util_spdy3.h (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy3...
net/spdy/spdy_test_util_spdy3.h:190: // Returns the constructed frame.  The
caller takes ownership of the frame.
here too

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_write_queue.cc
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_write_queue.cc#...
net/spdy/spdy_write_queue.cc:108: it = queue_[i].erase(it);
unfortunately, deque::erase is O(n). Can you do something similar to
RemovePendingWritesForStream where you keep track of *out_it and then delete
everything after that? May want to copy that loop above and replace the
conditional. I don't know if it's worth decomping out into a helper method.

[Ryan Hamilton, 2013-04-18 00:30:40]

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_session.cc#newc...
net/spdy/spdy_session.cc:1146: ActiveStreamMap new_active_streams;
On 2013/04/17 21:14:20, akalin wrote:
> Curious -- are we worried about re-entrancy here? If not, seems easier to use
> lower_bound to find the first stream > last_good_stream_id and start deleting
> from there

Done.

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_session.h#newco...
net/spdy/spdy_session.h:613: void CloseAllStreamsAfter(SpdyStreamId
last_good_stream_id,
On 2013/04/17 21:14:20, akalin wrote:
> add a comment for CloseAllStreamsAfter? Perhaps document the additional stuff
> CloseAllStreams does over CloseAllStreamsAfter(0, _)

Done.

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2.cc
File net/spdy/spdy_test_util_spdy2.cc (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2...
net/spdy/spdy_test_util_spdy2.cc:152: // Construct a SPDY GOAWAY frame.
On 2013/04/17 21:14:20, akalin wrote:
> i don't know if it's worth having the same comment in both the .h and the .cc
> file. But I guess it's consistent.

Yeah, I was surprised to see them too, but decided to be consistent.  However,
I've removed *all* these redundant comments.  Cool?

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2.h
File net/spdy/spdy_test_util_spdy2.h (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2...
net/spdy/spdy_test_util_spdy2.h:119: // Construct a SPDY GOAWAY frame.
On 2013/04/17 21:14:20, akalin wrote:
> document difference between this overload and the previous one?

Done.

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy3.cc
File net/spdy/spdy_test_util_spdy3.cc (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy3...
net/spdy/spdy_test_util_spdy3.cc:206: // Construct a SPDY GOAWAY frame.
On 2013/04/17 21:14:20, akalin wrote:
> here too

ditto :>

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy3.h
File net/spdy/spdy_test_util_spdy3.h (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy3...
net/spdy/spdy_test_util_spdy3.h:190: // Returns the constructed frame.  The
caller takes ownership of the frame.
On 2013/04/17 21:14:20, akalin wrote:
> here too

ditto

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_write_queue.cc
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_write_queue.cc#...
net/spdy/spdy_write_queue.cc:108: it = queue_[i].erase(it);
On 2013/04/17 21:14:20, akalin wrote:
> unfortunately, deque::erase is O(n). Can you do something similar to
> RemovePendingWritesForStream where you keep track of *out_it and then delete
> everything after that? May want to copy that loop above and replace the
> conditional. I don't know if it's worth decomping out into a helper method.

Done.

[akalin, 2013-04-18 01:29:59]

lgtm after nits

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2.cc
File net/spdy/spdy_test_util_spdy2.cc (right):

https://codereview.chromium.org/14232014/diff/1/net/spdy/spdy_test_util_spdy2...
net/spdy/spdy_test_util_spdy2.cc:152: // Construct a SPDY GOAWAY frame.
On 2013/04/18 00:30:40, Ryan Hamilton wrote:
> On 2013/04/17 21:14:20, akalin wrote:
> > i don't know if it's worth having the same comment in both the .h and the
.cc
> > file. But I guess it's consistent.
> 
> Yeah, I was surprised to see them too, but decided to be consistent.  However,
> I've removed *all* these redundant comments.  Cool?

even better!

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1151: if (stream->stream_id() <= last_good_stream_id) {
you don't need this check, right? Since ActiveStreamMap is ordered.

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1156: streams_abandoned_count_++;
hunh. we weren't incrementing this correctly? i see.

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1170: void SpdySession::CloseAllStreams(net::Error
status) {
please put this above to match order in header (or change the header order)

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_test_util_sp...
File net/spdy/spdy_test_util_spdy2.h (right):

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_test_util_sp...
net/spdy/spdy_test_util_spdy2.h:119: // Construct a SPDY GOAWAY frame with the
specified last_good_stream_id..
remove extra dot

[akalin, 2013-04-18 02:23:10]

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_write_queue.h
File net/spdy/spdy_write_queue.h (right):

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_write_queue....
net/spdy/spdy_write_queue.h:52: void
RemovePendingWritesForStreamsAfter(SpdyStreamId last_good_stream_id);
before i forget, can you also add a unit test for this?

[Ryan Hamilton, 2013-04-18 17:18:29]

Done, I think, but I'd like to confirm that it is acceptable for streams write
to be enqueued in the write queue even if they don't have a stream id yet.

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1151: if (stream->stream_id() <= last_good_stream_id) {
On 2013/04/18 01:29:59, akalin wrote:
> you don't need this check, right? Since ActiveStreamMap is ordered.

Hah!  Yes :>  Done.

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1156: streams_abandoned_count_++;
On 2013/04/18 01:29:59, akalin wrote:
> hunh. we weren't incrementing this correctly? i see.

Before I got started?  I think we were doing something like:

abandoned_streams.Add(active_streams_.size());

At least that was the state when I started hacking.  Looks like the name of that
var changed in a recent CL.

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:1170: void SpdySession::CloseAllStreams(net::Error
status) {
On 2013/04/18 01:29:59, akalin wrote:
> please put this above to match order in header (or change the header order)

Done.

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_test_util_sp...
File net/spdy/spdy_test_util_spdy2.h (right):

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_test_util_sp...
net/spdy/spdy_test_util_spdy2.h:119: // Construct a SPDY GOAWAY frame with the
specified last_good_stream_id..
On 2013/04/18 01:29:59, akalin wrote:
> remove extra dot

Done.

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_write_queue.h
File net/spdy/spdy_write_queue.h (right):

https://codereview.chromium.org/14232014/diff/4002/net/spdy/spdy_write_queue....
net/spdy/spdy_write_queue.h:52: void
RemovePendingWritesForStreamsAfter(SpdyStreamId last_good_stream_id);
On 2013/04/18 02:23:11, akalin wrote:
> before i forget, can you also add a unit test for this?

Done.

[akalin, 2013-04-18 19:58:19]

still lgtm, but a couple more nits

https://codereview.chromium.org/14232014/diff/15001/net/spdy/spdy_write_queue.cc
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/14232014/diff/15001/net/spdy/spdy_write_queue...
net/spdy/spdy_write_queue.cc:101: it->stream->stream_id() == 0)) {
ah, interesting. can you add to the header comment that we also delete writes
for streams with no ids?

https://codereview.chromium.org/14232014/diff/15001/net/spdy/spdy_write_queue...
File net/spdy/spdy_write_queue_unittest.cc (right):

https://codereview.chromium.org/14232014/diff/15001/net/spdy/spdy_write_queue...
net/spdy/spdy_write_queue_unittest.cc:212: << "Unable to Dequeue i: "<< i;
space before second <<

[Ryan Hamilton, 2013-04-18 21:33:46]

thanks

https://codereview.chromium.org/14232014/diff/15001/net/spdy/spdy_write_queue.cc
File net/spdy/spdy_write_queue.cc (right):

https://codereview.chromium.org/14232014/diff/15001/net/spdy/spdy_write_queue...
net/spdy/spdy_write_queue.cc:101: it->stream->stream_id() == 0)) {
On 2013/04/18 19:58:19, akalin wrote:
> ah, interesting. can you add to the header comment that we also delete writes
> for streams with no ids?

Done.

https://codereview.chromium.org/14232014/diff/15001/net/spdy/spdy_write_queue...
File net/spdy/spdy_write_queue_unittest.cc (right):

https://codereview.chromium.org/14232014/diff/15001/net/spdy/spdy_write_queue...
net/spdy/spdy_write_queue_unittest.cc:212: << "Unable to Dequeue i: "<< i;
On 2013/04/18 19:58:19, akalin wrote:
> space before second <<

Done.

[commit-bot: I haz the power, 2013-04-18 21:34:07]

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

[commit-bot: I haz the power, 2013-04-18 21:34:13]

Presubmit check for 14232014-23001 failed and returned exit status 1.

INFO:root:Found 11 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Traceback (most recent call last):
  File "/b/commit-queue/verification/presubmit_shim.py", line 33, in <module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1305, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1129, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1046, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 865, in CheckChangeOnCommit
  File "<string>", line 700, in _CommonChecks
  File "<string>", line 258, in _CheckNoUNIT_TESTInSourceFiles
  File "/b/depot_tools/presubmit_support.py", line 549, in ChangedContents
    for line in self.GenerateScmDiff().splitlines():
  File "/b/depot_tools/presubmit_support.py", line 619, in GenerateScmDiff
    [self.LocalPath()], self._local_root, False, None)
  File "/b/depot_tools/scm.py", line 913, in GenerateDiff
    full_move, revision))
  File "/b/depot_tools/scm.py", line 837, in _DiffItemInternal
    data = SVN.Capture(command, cwd)
  File "/b/depot_tools/scm.py", line 479, in Capture
    ['svn'] + args, stderr=subprocess2.PIPE, cwd=cwd, **kwargs)
  File "/b/depot_tools/subprocess2.py", line 449, in check_output
    return check_call_out(args, stdout=PIPE, **kwargs)[0]
  File "/b/depot_tools/subprocess2.py", line 412, in check_call_out
    returncode, args, kwargs.get('cwd'), out[0], out[1])
subprocess2.CalledProcessError: Command svn diff --internal-diff
net/spdy/spdy_session.cc returned non-zero exit status 1 in
/b/commit-queue/workdir/chromium
svn: invalid option: --internal-diff
Type 'svn help' for usage.

[commit-bot: I haz the power, 2013-04-18 21:40:52]

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

[commit-bot: I haz the power, 2013-04-18 21:40:59]

Presubmit check for 14232014-23001 failed and returned exit status 1.

INFO:root:Found 11 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Traceback (most recent call last):
  File "/b/commit-queue/verification/presubmit_shim.py", line 33, in <module>
    sys.exit(presubmit_support.Main(argv))
  File "/b/depot_tools/presubmit_support.py", line 1305, in Main
    rietveld_obj)
  File "/b/depot_tools/presubmit_support.py", line 1129, in DoPresubmitChecks
    results += executer.ExecPresubmitScript(presubmit_script, filename)
  File "/b/depot_tools/presubmit_support.py", line 1046, in ExecPresubmitScript
    result = eval(function_name + '(*__args)', context)
  File "<string>", line 1, in <module>
  File "<string>", line 865, in CheckChangeOnCommit
  File "<string>", line 700, in _CommonChecks
  File "<string>", line 258, in _CheckNoUNIT_TESTInSourceFiles
  File "/b/depot_tools/presubmit_support.py", line 549, in ChangedContents
    for line in self.GenerateScmDiff().splitlines():
  File "/b/depot_tools/presubmit_support.py", line 619, in GenerateScmDiff
    [self.LocalPath()], self._local_root, False, None)
  File "/b/depot_tools/scm.py", line 913, in GenerateDiff
    full_move, revision))
  File "/b/depot_tools/scm.py", line 837, in _DiffItemInternal
    data = SVN.Capture(command, cwd)
  File "/b/depot_tools/scm.py", line 479, in Capture
    ['svn'] + args, stderr=subprocess2.PIPE, cwd=cwd, **kwargs)
  File "/b/depot_tools/subprocess2.py", line 449, in check_output
    return check_call_out(args, stdout=PIPE, **kwargs)[0]
  File "/b/depot_tools/subprocess2.py", line 412, in check_call_out
    returncode, args, kwargs.get('cwd'), out[0], out[1])
subprocess2.CalledProcessError: Command svn diff --internal-diff
net/spdy/spdy_session.cc returned non-zero exit status 1 in
/b/commit-queue/workdir/chromium
svn: invalid option: --internal-diff
Type 'svn help' for usage.

[commit-bot: I haz the power, 2013-04-18 21:49:01]

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

[commit-bot: I haz the power, 2013-04-19 00:01:20]

Retried try job too often on mac_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2013-04-19 00:11:57]

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

[commit-bot: I haz the power, 2013-04-19 00:50:21]

Change committed as 195032