Implement PUSH_PROMISE handling in spdy_session

Implement PUSH_PROMISE handling in spdy_session

BUG=377538
R=jgraettinger@chromium.org

TEST=SpdyStream/SpdySession/SpdyNetworkTransaction push related tests.
     Re-enabled previously disabled tests:
     SpdyNetworkTransactionTest.ServerPushCrossOriginCorrectness
     SpdyNetworkTransactionTest.ServerPushInvalidAssociatedStreamID9

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

[baranovich, 2014-06-13 11:42:46]

Hi Johnny,
This CL introduces some PUSH_PROMISE handling.
Correct getting of push stream using all push promise headers will be in next
CLs.

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:3094: // PUSH_PROMISE with stream
id 0 is connection-level error.
Should I write a test about sending goaway? I think no, since it is
framer-initiated goaway, and is tested as every goaway is. But I may be wrong=)

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

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:562:
max_concurrent_streams_(initial_max_concurrent_streams == 0
its not me, its git cl format=)

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:783: reserved_remote_stream_num_ <
This behavior should be tested. But it is incorrect from the start, since we
count both client and push streams towards a single limit
|max_concurrent_streams_| which is set from server settings and should be a
limit of client initiated stream only. This is not only about SPDY4. Adding a
pushed stream to SpdyNetworkTransactionTest.*MaxConcurrent*  will break them all
=) So, I could:
1) write a test on this incorrect behavior
2) leave this code untested for now, and later implement correct stream limits
in separate CL
3) write that code after correct stream limits implementation.
What is better?

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_test_util_comm...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_test_util_comm...
net/spdy/spdy_test_util_common.cc:1018: SpdySynStreamIR headers(stream_id);
Doing it this way, I check codepath via OnSynStream(). But headers can come
directly from OnHeaders() (no priority). This is untested now. Should I write a
single test which will cover this case?

[Johnny, 2014-06-17 04:01:33]

Hi Alexey,

Nice job! Overall this is in good shape. Comments inline.

-johnny

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:3094: // PUSH_PROMISE with stream
id 0 is connection-level error.
On 2014/06/13 11:42:46, baranovich wrote:
> Should I write a test about sending goaway? I think no, since it is
> framer-initiated goaway, and is tested as every goaway is. But I may be
wrong=)

It'd be good for completeness, and regression testing that the invalid stream
maps to a GOAWAY / PROTOCOL_ERROR as required by the spec. Can be done later (a
TODO is fine).

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:3164:
spdy_util_.ConstructSpdyRstStream(2, RST_STREAM_INVALID_STREAM));
My read of the spec is that invalid stream identifiers are a
GOAWAY/PROTOCOL_ERROR (under "promised stream ID" in section 6.6, and then
section 5.1.1).

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

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:562:
max_concurrent_streams_(initial_max_concurrent_streams == 0
On 2014/06/13 11:42:46, baranovich wrote:
> its not me, its git cl format=)

Yep, not a problem.

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:783: reserved_remote_stream_num_ <
On 2014/06/13 11:42:46, baranovich wrote:
> This behavior should be tested. But it is incorrect from the start, since we
> count both client and push streams towards a single limit
> |max_concurrent_streams_| which is set from server settings and should be a
> limit of client initiated stream only. This is not only about SPDY4. Adding a
> pushed stream to SpdyNetworkTransactionTest.*MaxConcurrent*  will break them
all
> =) So, I could:
> 1) write a test on this incorrect behavior
> 2) leave this code untested for now, and later implement correct stream limits
> in separate CL
> 3) write that code after correct stream limits implementation.
> What is better?

Agreed with your assessment. The current behavior is incorrect, but it's been
consistently incorrect for some time now: I think I prefer option 3), and
leaving as the status quo with a TODO until it can be done properly.

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:2137: if
(active_it->second.stream->OnPushPromiseHeadersReceived(request_headers) !=
It seems like TryCreatePushStream() should receive |request_headers|, and should
make the call to OnPushPromiseHeadersReceived().

Also, it should abort adding to the stream to |active_streams_| if
OnPushPromiseHeadersReceived() returns != OK (and also probably call
DoDrainSession()),  or OnPushPromiseHeadersReceived() should just return void
(as it currently does nothing but return OK).

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:2155: DCHECK_GT(reserved_remote_stream_num_, 0u);
I'm thinking this ought to be folded into OnResponseHeadersReceived(), as a
check of "IsReservedRemote()" prior to calling
stream->OnResponseHeadersReceived().

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

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.h#newc...
net/spdy/spdy_session.h:540: bool reserved_remote;
Rather than having |reserved_remote| here, add an "IsReservedRemote()" accessor
which queries stream's |io_state_|.

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.h#newc...
net/spdy/spdy_session.h:978: size_t reserved_remote_stream_num_;
How about |num_reserved_remote_streams_| to match
num_active_streams()/num_created_streams()/etc convention used elsewhere. Needs
a good comment about what this is and why it's tracked.

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

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_stream.cc#newc...
net/spdy/spdy_stream.cc:126: // implemented.
Remove TODO

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_test_util_comm...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_test_util_comm...
net/spdy/spdy_test_util_common.cc:1018: SpdySynStreamIR headers(stream_id);
On 2014/06/13 11:42:46, baranovich wrote:
> Doing it this way, I check codepath via OnSynStream(). But headers can come
> directly from OnHeaders() (no priority). This is untested now. Should I write
a
> single test which will cover this case?

I don't think I fully follow, but as a rule of thumb for SpdyTestUtil: if it's
not necessary to get tests passing, better to leave as-is and incomplete for
SPDY4 than to add implementation which tries to fix it, but doesn't have tests
exercising it.

[baranovich, 2014-06-17 21:33:43]

All done, except for promised stream id error handling. Your advice needed=)

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:3094: // PUSH_PROMISE with stream
id 0 is connection-level error.
On 2014/06/17 04:01:32, Johnny wrote:
> On 2014/06/13 11:42:46, baranovich wrote:
> > Should I write a test about sending goaway? I think no, since it is
> > framer-initiated goaway, and is tested as every goaway is. But I may be
> wrong=)
> 
> It'd be good for completeness, and regression testing that the invalid stream
> maps to a GOAWAY / PROTOCOL_ERROR as required by the spec. Can be done later
(a
> TODO is fine).

Done.

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:3164:
spdy_util_.ConstructSpdyRstStream(2, RST_STREAM_INVALID_STREAM));
True, but that's true for all frame types.
But AFAIU this test is not about promised stream id, but about associated stream
id. We'd better not go away on PUSH_PROMISE with invalid (associated)stream id,
because of races (stream is locally cancelled and deleted but lots of data from
the server (including push promises) may be still in the wire). And we still
should send RST_STREAM for promised-stream-id to the server. So to my mind, this
behavior is correct although it is not directly in spec.

For unexpected promised stream id (odd id, less than last seen id, duplicate
id), I agree, we should go away.
Although from SPDY 3 spec
(http://www.chromium.org/spdy/spdy-protocol/spdy-protocol-draft3-1#TOC-2.3.2-S...):
The stream-id MUST increase with each new stream. If an endpoint receives a
SYN_STREAM with a stream id which is less than any previously received
SYN_STREAM, it MUST issue a session error (Section 2.4.1) with the status
PROTOCOL_ERROR.

It is a protocol error to send two SYN_STREAMs with the same stream-id. If a
recipient receives a second SYN_STREAM for the same stream, it MUST issue a
stream error Section (2.4.2) with the status code PROTOCOL_ERROR.

Not a word about what to do with odd ids, but still... this is not implemented,
we just do nothing silently.

Should I implement all this for SPDY4 and SPDY3 then?

On 2014/06/17 04:01:32, Johnny wrote:
> My read of the spec is that invalid stream identifiers are a
> GOAWAY/PROTOCOL_ERROR (under "promised stream ID" in section 6.6, and then
> section 5.1.1).

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

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:783: reserved_remote_stream_num_ <
On 2014/06/17 04:01:33, Johnny wrote:
> On 2014/06/13 11:42:46, baranovich wrote:
> > This behavior should be tested. But it is incorrect from the start, since we
> > count both client and push streams towards a single limit
> > |max_concurrent_streams_| which is set from server settings and should be a
> > limit of client initiated stream only. This is not only about SPDY4. Adding
a
> > pushed stream to SpdyNetworkTransactionTest.*MaxConcurrent*  will break them
> all
> > =) So, I could:
> > 1) write a test on this incorrect behavior
> > 2) leave this code untested for now, and later implement correct stream
limits
> > in separate CL
> > 3) write that code after correct stream limits implementation.
> > What is better?
> 
> Agreed with your assessment. The current behavior is incorrect, but it's been
> consistently incorrect for some time now: I think I prefer option 3), and
> leaving as the status quo with a TODO until it can be done properly.

Done. Left this code intact

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:2137: if
(active_it->second.stream->OnPushPromiseHeadersReceived(request_headers) !=
On 2014/06/17 04:01:33, Johnny wrote:
> It seems like TryCreatePushStream() should receive |request_headers|, and
should
> make the call to OnPushPromiseHeadersReceived().
> 
> Also, it should abort adding to the stream to |active_streams_| if
> OnPushPromiseHeadersReceived() returns != OK (and also probably call
> DoDrainSession()),  or OnPushPromiseHeadersReceived() should just return void
> (as it currently does nothing but return OK).

Done.

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.cc#new...
net/spdy/spdy_session.cc:2155: DCHECK_GT(reserved_remote_stream_num_, 0u);
On 2014/06/17 04:01:33, Johnny wrote:
> I'm thinking this ought to be folded into OnResponseHeadersReceived(), as a
> check of "IsReservedRemote()" prior to calling
> stream->OnResponseHeadersReceived().

Done. This method in not necessary unless we have reserved stream counter

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

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.h#newc...
net/spdy/spdy_session.h:540: bool reserved_remote;
On 2014/06/17 04:01:33, Johnny wrote:
> Rather than having |reserved_remote| here, add an "IsReservedRemote()"
accessor
> which queries stream's |io_state_|.

Done.

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_session.h#newc...
net/spdy/spdy_session.h:978: size_t reserved_remote_stream_num_;
On 2014/06/17 04:01:33, Johnny wrote:
> How about |num_reserved_remote_streams_| to match
> num_active_streams()/num_created_streams()/etc convention used elsewhere.
Needs
> a good comment about what this is and why it's tracked.

Done. Deleted.

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

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_stream.cc#newc...
net/spdy/spdy_stream.cc:126: // implemented.
On 2014/06/17 04:01:33, Johnny wrote:
> Remove TODO

Done.

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_test_util_comm...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_test_util_comm...
net/spdy/spdy_test_util_common.cc:1018: SpdySynStreamIR headers(stream_id);
On 2014/06/17 04:01:33, Johnny wrote:
> On 2014/06/13 11:42:46, baranovich wrote:
> > Doing it this way, I check codepath via OnSynStream(). But headers can come
> > directly from OnHeaders() (no priority). This is untested now. Should I
write
> a
> > single test which will cover this case?
> 
> I don't think I fully follow, but as a rule of thumb for SpdyTestUtil: if it's
> not necessary to get tests passing, better to leave as-is and incomplete for
> SPDY4 than to add implementation which tries to fix it, but doesn't have tests
> exercising it.

Server may send HEADERS with or without priority. If priority flag is set, push
'response' will go via OnSynStream() (which will delegate to OnHeaders()),
otherwise we'll directly get to OnHeaders(). I am worried about the second case
which is not covered by tests (well, covered, but not a direct call=)). But its
minor I think.

[Johnny, 2014-06-19 16:58:52]

lgtm

Nice job!

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
File net/spdy/spdy_network_transaction_unittest.cc (right):

https://codereview.chromium.org/331663007/diff/1/net/spdy/spdy_network_transa...
net/spdy/spdy_network_transaction_unittest.cc:3164:
spdy_util_.ConstructSpdyRstStream(2, RST_STREAM_INVALID_STREAM));
On 2014/06/17 21:33:42, baranovich wrote:
> ... We'd better not go away on PUSH_PROMISE with invalid (associated)stream
id,
> because of races (stream is locally cancelled and deleted but lots of data
from
> the server (including push promises) may be still in the wire).

That's an excellent point. I'm convinced. So long as we immediately "forget"
about streams which we've cancelled, we need this behavior.


> Not a word about what to do with odd ids, but still... this is not
implemented,
> we just do nothing silently.
> 
> Should I implement all this for SPDY4 and SPDY3 then?

Yep, I put another comment on the spot where we do this in parallel, before
reading this. It'd be good to do this later, but this CL is ready to land as-is.

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

https://codereview.chromium.org/331663007/diff/60001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:2100: if (GetProtocolVersion() >= SPDY4) {
Optional Nit: This could also live in BufferedSpdyFramer, which is already set
up as the "translation layer" for SpdyFramer.

https://codereview.chromium.org/331663007/diff/60001/net/spdy/spdy_session.cc...
net/spdy/spdy_session.cc:2439: LOG(WARNING) << "Received push for active stream
" << stream_id;
Shouldn't be done in this CL, but we probably eventually want this to be a
GOAWAY / PROTOCOL_ERROR.

[baranovich, 2014-06-20 09:00:10]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-06-20 09:01:33]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/331663007/60001

[baranovich, 2014-06-20 12:51:27]

The CQ bit was checked by baranovich@yandex-team.ru

[commit-bot: I haz the power, 2014-06-20 12:57:20]

CQ is trying da patch. Follow status at

https://chromium-status.appspot.com/cq/baranovich@yandex-team.ru/331663007/80001

[commit-bot: I haz the power, 2014-06-20 17:10:04]

Change committed as 278733