Change the stream_id for streams which are serialized out of order.

Change the stream_id for streams which are serialized out of order.

BUG=111708
TEST=SpdySessionSpdy\*Test.OutOfOrderSynStreams


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=137180

[Ryan Hamilton, 2012-05-10 22:45:53]

Hi Guys,

Here's my CL to fix the out of order SYN_STREAM frames.  I'm not exactly in love
with constructing a SpdyControlFrame and then a SpdySynStreamFrame in
WriteSocket, but it seemed the most straightforward.  What do y'all think?

I also need to tweak the logging to make it clear that we have re-numbered a
stream, assuming that this approach is kosher.

Cheers,

Ryan

[willchan no longer on Chromium, 2012-05-10 23:56:41]

Thanks for tackling this Ryan.

After looking at this further, I think the original architecture (no fault of
your own!) is bad. I think this OutputQueue is wrong. It's a buffer and we
shouldn't have unlimited buffering in our code. In the same way we use epoll()
patterns to check for writability to a socket and invoke callbacks, we should
detect writability to the SPDY socket, and when it's writable, check our
SpdyStreams and PendingCreateStreams to see which one we want to create.

WDYT? That would require a larger refactor (but I believe still quite
tractable), which I don't want to block your change on. I think I'd be OK with
doing the minimal necessary now to get it to work in a way that doesn't hurt
perf, and then address this architectural issue later.

Your method right now is a bit sketchy, but I think I see why you do it. And if
we can get it done correctly, then I'm fine with it.

[Ryan Hamilton, 2012-05-14 15:33:18]

On 2012/05/10 23:56:41, willchan wrote:
> Thanks for tackling this Ryan.
> 
> After looking at this further, I think the original architecture (no fault of
> your own!) is bad. I think this OutputQueue is wrong. It's a buffer and we
> shouldn't have unlimited buffering in our code. In the same way we use epoll()
> patterns to check for writability to a socket and invoke callbacks, we should
> detect writability to the SPDY socket, and when it's writable, check our
> SpdyStreams and PendingCreateStreams to see which one we want to create.
> 
> WDYT? That would require a larger refactor (but I believe still quite
> tractable), which I don't want to block your change on. I think I'd be OK with
> doing the minimal necessary now to get it to work in a way that doesn't hurt
> perf, and then address this architectural issue later.

Sounds reasonable.  It seems that the idea would be to switch from a queue of
frames to a queue of streams (with writes pending).  Definitely non-trivial, but
seems totally reasonable.  I'll get to work.

> Your method right now is a bit sketchy, but I think I see why you do it. And
if
> we can get it done correctly, then I'm fine with it.

*nod*  Given that the current behavior is buggy, and my proposed CL seems pretty
small (albeit a bit ugly) I'd like to recommend that we land this CL (and merge
it up to m20 once we're sure it is stable).  We'll target m21 to release the
refactor you propose.  Work for you?

[willchan no longer on Chromium, 2012-05-14 15:38:31]

I agree. I'll review this today once I get through some meetings. I don't
expect to have large comments.

On Mon, May 14, 2012 at 11:33 AM, <rch@chromium.org> wrote:

> On 2012/05/10 23:56:41, willchan wrote:
>
>> Thanks for tackling this Ryan.
>>
>
>  After looking at this further, I think the original architecture (no
>> fault of
>> your own!) is bad. I think this OutputQueue is wrong. It's a buffer and we
>> shouldn't have unlimited buffering in our code. In the same way we use
>> epoll()
>> patterns to check for writability to a socket and invoke callbacks, we
>> should
>> detect writability to the SPDY socket, and when it's writable, check our
>> SpdyStreams and PendingCreateStreams to see which one we want to create.
>>
>
>  WDYT? That would require a larger refactor (but I believe still quite
>> tractable), which I don't want to block your change on. I think I'd be OK
>> with
>> doing the minimal necessary now to get it to work in a way that doesn't
>> hurt
>> perf, and then address this architectural issue later.
>>
>
> Sounds reasonable.  It seems that the idea would be to switch from a queue
> of
> frames to a queue of streams (with writes pending).  Definitely
> non-trivial, but
> seems totally reasonable.  I'll get to work.
>
>
>  Your method right now is a bit sketchy, but I think I see why you do it.
>> And
>>
> if
>
>> we can get it done correctly, then I'm fine with it.
>>
>
> *nod*  Given that the current behavior is buggy, and my proposed CL seems
> pretty
> small (albeit a bit ugly) I'd like to recommend that we land this CL (and
> merge
> it up to m20 once we're sure it is stable).  We'll target m21 to release
> the
> refactor you propose.  Work for you?
>
>
>
>
http://codereview.chromium.**org/10382107/<http://codereview.chromium.org/103...
>

[willchan no longer on Chromium, 2012-05-15 13:54:15]

lgtm

http://codereview.chromium.org/10382107/diff/6001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/10382107/diff/6001/net/spdy/spdy_session.cc#ne...
net/spdy/spdy_session.cc:679: NetLog::TYPE_SPDY_SESSION_SYN_STREAM,
This is going to be off now...right? Maybe we just need to temporarily know this
is wrong? Long-term, we'll fix this to be right...perhaps what we should do
temporarily is add a new event that says we're rewriting the stream id at the
last second and indicate the old and new stream ids?

[willchan no longer on Chromium, 2012-05-15 14:04:29]

http://codereview.chromium.org/10382107/diff/6001/net/spdy/spdy_stream.cc
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/10382107/diff/6001/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:524: DCHECK(io_state_ > STATE_SEND_HEADERS_COMPLETE);
Btw, does DCHECK_GT work here?

[Ryan Hamilton, 2012-05-15 16:41:11]

I've addressed your comments and will commit.  Thanks!

https://chromiumcodereview.appspot.com/10382107/diff/6001/net/spdy/spdy_sessi...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/10382107/diff/6001/net/spdy/spdy_sessi...
net/spdy/spdy_session.cc:679: NetLog::TYPE_SPDY_SESSION_SYN_STREAM,
On 2012/05/15 13:54:15, willchan wrote:
> This is going to be off now...right? Maybe we just need to temporarily know
this
> is wrong? Long-term, we'll fix this to be right...perhaps what we should do
> temporarily is add a new event that says we're rewriting the stream id at the
> last second and indicate the old and new stream ids?

Oh, right.  I had though about this, but apparently neglected to implement. 
I've added a new event type for renumbering a SPDY stream, and have tweaked this
CL accordingly.

https://chromiumcodereview.appspot.com/10382107/diff/6001/net/spdy/spdy_strea...
File net/spdy/spdy_stream.cc (right):

https://chromiumcodereview.appspot.com/10382107/diff/6001/net/spdy/spdy_strea...
net/spdy/spdy_stream.cc:524: DCHECK(io_state_ > STATE_SEND_HEADERS_COMPLETE);
On 2012/05/15 14:04:30, willchan wrote:
> Btw, does DCHECK_GT work here?

Done.

[commit-bot: I haz the power, 2012-05-15 16:41:26]

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

[commit-bot: I haz the power, 2012-05-15 18:23:45]

Change committed as 137180