Fix bug where pushed SPDY streams were never actually closed....

Fix bug where pushed SPDY streams were never actually closed.

The fix uncovers a few things.  Previously, when we had an unclaimed
push stream, we'd leave it as an "active" stream, even if the EOF had
already been received on that stream.  I changed it so that the push
stream is removed from the active stream as soon as it is inactive,
but it remains on the unclaimed_pushed_streams list.  This seems more
correct.

The hardest part of the test was getting the test to verify the fix.
To verify the fix, I modified the Push tests so that we leave the
session open (e.g. terminate the list of reads with an ERR_IO_PENDING
rather than an EOF so that it sits open), which gives us the opportunity
to check if there are active streams left on the session when we 
completed the test.  If so, then we'll pop an error.

BUG=52898
TEST=Updated existing tests.



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

[willchan no longer on Chromium, 2010-08-22 16:20:49]

Everything works and LGTM as is, but I've made a number of suggestions which I
think we should probably implement, but don't necessarily have to be done in
this changelist.

http://codereview.chromium.org/3108038/diff/14001/15002
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15002#newcode337
net/spdy/spdy_network_transaction_unittest.cc:337: std::string
result1(buf->data(), rv);
This result1 is unnecessary.  Just do result->append(buf->data(), rv);

http://codereview.chromium.org/3108038/diff/14001/15002#newcode398
net/spdy/spdy_network_transaction_unittest.cc:398: {
Nit: Is there a reason you need to scope this?  Is it to make it clear what the
code is doing?  It'd probably be better to write a GetSpdySessionForURL()
function or something, rather than writing a lengthy comment.  That's just how I
would do it, but feel free to do it this way if you prefer.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode400
net/spdy/spdy_network_transaction_unittest.cc:400: // session.  Once we have the
sesion, we verify that the streams are
session

http://codereview.chromium.org/3108038/diff/14001/15002#newcode402
net/spdy/spdy_network_transaction_unittest.cc:402: GURL url =
helper.request().url;
const GURL&

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2178
net/spdy/spdy_network_transaction_unittest.cc:2178: 'p', 'u', 's', 'h', 'e', 'd'
                                // "hello"
update comment

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2186
net/spdy/spdy_network_transaction_unittest.cc:2186: scoped_ptr<spdy::SpdyFrame>
resp(ConstructSpdyGetSynReply(NULL, 0, 1));
Meta-comment: these variable names suck.  resp and rep?  It's confusing.  I
think of resp == response and rep == reply.  But It's not obvious what the
responses/replies are to.  Nothing you have to fix in this changelist, but I
thought I'd mention it.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2191
net/spdy/spdy_network_transaction_unittest.cc:2191: CreateMockRead(*body.get(),
4, false),
Is this a bugfix?  Is it because we don't have a FIN on resp?

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2216
net/spdy/spdy_network_transaction_unittest.cc:2216: 'p', 'u', 's', 'h', 'e', 'd'
                                // "hello"
update comment

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2264
net/spdy/spdy_network_transaction_unittest.cc:2264: 'p', 'u', 's', 'h', 'e', 'd'
                                // "hello"
update comment

http://codereview.chromium.org/3108038/diff/14001/15003
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15003#newcode201
net/spdy/spdy_session.cc:201: RecordHistograms();
I think that at the end of SpdySession(), we should DCHECK all the stream lists
to make sure they're empty.  CloseAllStreams() should have cleared them all out
by this point.

http://codereview.chromium.org/3108038/diff/14001/15003#newcode781
net/spdy/spdy_session.cc:781: if (!unclaimed_pushed_streams_.empty()) {
Should we also do unclaimed_pushed_streams_.clear(); here?  Otherwise, we're not
closing all streams.

http://codereview.chromium.org/3108038/diff/14001/15004
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/3108038/diff/14001/15004#newcode167
net/spdy/spdy_session.h:167: size_t num_active_streams() { return
active_streams_.size(); }
These new accessors should be const.

http://codereview.chromium.org/3108038/diff/14001/15005
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15005#newcode189
net/spdy/spdy_stream.cc:189: scoped_refptr<SpdyStream> self(this);
Can we avoid this self-reference?  It looks like you do it because CloseStream()
could remove the last reference and delete the object, which would cause badness
in UpdateHistograms().  Can we move UpdateHistograms into the destructor?  Or
does that cause problems since we might be updating the histograms too late? 
It'd also remove the need to have the boolean that checks to make sure we don't
update the histograms more than once per stream.

http://codereview.chromium.org/3108038/diff/14001/15005#newcode190
net/spdy/spdy_stream.cc:190: session_->CloseStream(stream_id_, net::OK);
Now that we're closing the stream here, which removes it from the active
streams, a subsequent CloseAllStreams() won't call OnClose() on this stream,
since it's only in the unclaimed push streams list.  Perhaps you need have
DeleteStream() or CloseAllStreams() do this.  Not calling OnClose() is currently
benign, but seems "wrong", since you'd expect that CloseAllStreams() would call
OnClose() on all streams.  I'm afraid that it's possible for a future refactor
to introduce a bug here.

[erikchen, 2010-08-22 17:40:05]

Hey Mike.
So the reason I didn't want to remove the pushed stream from active_streams_
until the 0 data read gets reread is that would make OnClose get called at the
right time. Under the current api, OnClose gets called only and exactly when the
stream is removed from active_streams_. I guess the current tradeoff is between
accuracy of the abstraction (should active_streams_ contain a pushed stream that
has theoretically been told to finish by the server) and centralization of code.
Your call.
Another thought. Not for this CL, but an important fix for SPDY tests I never
added. Every test that uses the spdy_session_pool should check at the end that
the pool is empty. Otherwise it is possible that a spdysession will leak and we
won't know about it (unless a future test calls OnIPAddressChanged, and gets a
weird error)

On 2010/08/22 16:20:49, willchan wrote:
> Everything works and LGTM as is, but I've made a number of suggestions which I
> think we should probably implement, but don't necessarily have to be done in
> this changelist.
> 
> http://codereview.chromium.org/3108038/diff/14001/15002
> File net/spdy/spdy_network_transaction_unittest.cc (right):
> 
> http://codereview.chromium.org/3108038/diff/14001/15002#newcode337
> net/spdy/spdy_network_transaction_unittest.cc:337: std::string
> result1(buf->data(), rv);
> This result1 is unnecessary.  Just do result->append(buf->data(), rv);
> 
> http://codereview.chromium.org/3108038/diff/14001/15002#newcode398
> net/spdy/spdy_network_transaction_unittest.cc:398: {
> Nit: Is there a reason you need to scope this?  Is it to make it clear what
the
> code is doing?  It'd probably be better to write a GetSpdySessionForURL()
> function or something, rather than writing a lengthy comment.  That's just how
I
> would do it, but feel free to do it this way if you prefer.
> 
> http://codereview.chromium.org/3108038/diff/14001/15002#newcode400
> net/spdy/spdy_network_transaction_unittest.cc:400: // session.  Once we have
the
> sesion, we verify that the streams are
> session
> 
> http://codereview.chromium.org/3108038/diff/14001/15002#newcode402
> net/spdy/spdy_network_transaction_unittest.cc:402: GURL url =
> helper.request().url;
> const GURL&
> 
> http://codereview.chromium.org/3108038/diff/14001/15002#newcode2178
> net/spdy/spdy_network_transaction_unittest.cc:2178: 'p', 'u', 's', 'h', 'e',
'd'
>                                 // "hello"
> update comment
> 
> http://codereview.chromium.org/3108038/diff/14001/15002#newcode2186
> net/spdy/spdy_network_transaction_unittest.cc:2186:
scoped_ptr<spdy::SpdyFrame>
> resp(ConstructSpdyGetSynReply(NULL, 0, 1));
> Meta-comment: these variable names suck.  resp and rep?  It's confusing.  I
> think of resp == response and rep == reply.  But It's not obvious what the
> responses/replies are to.  Nothing you have to fix in this changelist, but I
> thought I'd mention it.
> 
> http://codereview.chromium.org/3108038/diff/14001/15002#newcode2191
> net/spdy/spdy_network_transaction_unittest.cc:2191:
CreateMockRead(*body.get(),
> 4, false),
> Is this a bugfix?  Is it because we don't have a FIN on resp?
> 
> http://codereview.chromium.org/3108038/diff/14001/15002#newcode2216
> net/spdy/spdy_network_transaction_unittest.cc:2216: 'p', 'u', 's', 'h', 'e',
'd'
>                                 // "hello"
> update comment
> 
> http://codereview.chromium.org/3108038/diff/14001/15002#newcode2264
> net/spdy/spdy_network_transaction_unittest.cc:2264: 'p', 'u', 's', 'h', 'e',
'd'
>                                 // "hello"
> update comment
> 
> http://codereview.chromium.org/3108038/diff/14001/15003
> File net/spdy/spdy_session.cc (right):
> 
> http://codereview.chromium.org/3108038/diff/14001/15003#newcode201
> net/spdy/spdy_session.cc:201: RecordHistograms();
> I think that at the end of SpdySession(), we should DCHECK all the stream
lists
> to make sure they're empty.  CloseAllStreams() should have cleared them all
out
> by this point.
> 
> http://codereview.chromium.org/3108038/diff/14001/15003#newcode781
> net/spdy/spdy_session.cc:781: if (!unclaimed_pushed_streams_.empty()) {
> Should we also do unclaimed_pushed_streams_.clear(); here?  Otherwise, we're
not
> closing all streams.
> 
> http://codereview.chromium.org/3108038/diff/14001/15004
> File net/spdy/spdy_session.h (right):
> 
> http://codereview.chromium.org/3108038/diff/14001/15004#newcode167
> net/spdy/spdy_session.h:167: size_t num_active_streams() { return
> active_streams_.size(); }
> These new accessors should be const.
> 
> http://codereview.chromium.org/3108038/diff/14001/15005
> File net/spdy/spdy_stream.cc (right):
> 
> http://codereview.chromium.org/3108038/diff/14001/15005#newcode189
> net/spdy/spdy_stream.cc:189: scoped_refptr<SpdyStream> self(this);
> Can we avoid this self-reference?  It looks like you do it because
CloseStream()
> could remove the last reference and delete the object, which would cause
badness
> in UpdateHistograms().  Can we move UpdateHistograms into the destructor?  Or
> does that cause problems since we might be updating the histograms too late? 
> It'd also remove the need to have the boolean that checks to make sure we
don't
> update the histograms more than once per stream.
> 
> http://codereview.chromium.org/3108038/diff/14001/15005#newcode190
> net/spdy/spdy_stream.cc:190: session_->CloseStream(stream_id_, net::OK);
> Now that we're closing the stream here, which removes it from the active
> streams, a subsequent CloseAllStreams() won't call OnClose() on this stream,
> since it's only in the unclaimed push streams list.  Perhaps you need have
> DeleteStream() or CloseAllStreams() do this.  Not calling OnClose() is
currently
> benign, but seems "wrong", since you'd expect that CloseAllStreams() would
call
> OnClose() on all streams.  I'm afraid that it's possible for a future refactor
> to introduce a bug here.

[erikchen, 2010-08-22 17:40:45]

http://codereview.chromium.org/3108038/diff/14001/15002
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15002#newcode410
net/spdy/spdy_network_transaction_unittest.cc:410: scoped_refptr<SpdySession>
spdy_session(pool->Get(pair, session, log));
Doesn't pool->Get create a session, if it doesn't already exist? you probably
want to ASSERT_TRUE(pool->HasSession(pair)).

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2291
net/spdy/spdy_network_transaction_unittest.cc:2291: MockRead(true,
ERR_IO_PENDING, 9)  // Force a pause.
s/9/10

http://codereview.chromium.org/3108038/diff/14001/15003
File net/spdy/spdy_session.cc (left):

http://codereview.chromium.org/3108038/diff/14001/15003#oldcode866
net/spdy/spdy_session.cc:866: }
You may want to consider the edge case where a server pushes a stream, and then
sends a RST_STREAM for that stream. Shouldn't that remove the pushed stream as
well? Perhaps that can be moved to a separate function.

http://codereview.chromium.org/3108038/diff/14001/15003
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15003#newcode781
net/spdy/spdy_session.cc:781: if (!unclaimed_pushed_streams_.empty()) {
It was previously the case that all streams in unclaimed_pushed_ were also
active streams, and deleting the active stream would also delete the
unclaimed_pushed stream. This is unclear and should at the very least be
commented, or else changed to be more clear.
On 2010/08/22 16:20:50, willchan wrote:
> Should we also do unclaimed_pushed_streams_.clear(); here?  Otherwise, we're
not
> closing all streams.

http://codereview.chromium.org/3108038/diff/14001/15005
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15005#newcode189
net/spdy/spdy_stream.cc:189: scoped_refptr<SpdyStream> self(this);
I remember moving UpdateHistograms() into the destructor in some CL that got
lost in the annals of time. You have to add a bool that gets updated when we
start recording histograms, because calling UpdateHistograms() before calling
StartHisotgrams(), or whatever that call is, will result in a CHECK. 
I don't _think_ there was a problem with calling UpdateHistograms too late.


On 2010/08/22 16:20:50, willchan wrote:
> Can we avoid this self-reference?  It looks like you do it because
CloseStream()
> could remove the last reference and delete the object, which would cause
badness
> in UpdateHistograms().  Can we move UpdateHistograms into the destructor?  Or
> does that cause problems since we might be updating the histograms too late? 
> It'd also remove the need to have the boolean that checks to make sure we
don't
> update the histograms more than once per stream.

http://codereview.chromium.org/3108038/diff/14001/15005#newcode190
net/spdy/spdy_stream.cc:190: session_->CloseStream(stream_id_, net::OK);
Actually, the current implementation of CloseStream() does in fact call
OnClose() right here, and if memory serves me, OnClose() removes the delegate,
and does all that fun stuff. 
Now, the ability to remove an ActiveStream without calling OnClose would result
in precisely the problem willchan mentioned, which is why the current API does
not allow for that.

On 2010/08/22 16:20:50, willchan wrote:
> Now that we're closing the stream here, which removes it from the active
> streams, a subsequent CloseAllStreams() won't call OnClose() on this stream,
> since it's only in the unclaimed push streams list.  Perhaps you need have
> DeleteStream() or CloseAllStreams() do this.  Not calling OnClose() is
currently
> benign, but seems "wrong", since you'd expect that CloseAllStreams() would
call
> OnClose() on all streams.  I'm afraid that it's possible for a future refactor
> to introduce a bug here.

[Mike Belshe, 2010-08-22 21:52:29]

thanks guys for the quick, weekend review!

http://codereview.chromium.org/3108038/diff/14001/15002
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15002#newcode337
net/spdy/spdy_network_transaction_unittest.cc:337: std::string
result1(buf->data(), rv);
On 2010/08/22 16:20:50, willchan wrote:
> This result1 is unnecessary.  Just do result->append(buf->data(), rv);

Done.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode398
net/spdy/spdy_network_transaction_unittest.cc:398: {
Broke it out into a function.

On 2010/08/22 16:20:50, willchan wrote:
> Nit: Is there a reason you need to scope this?  Is it to make it clear what
the
> code is doing?  It'd probably be better to write a GetSpdySessionForURL()
> function or something, rather than writing a lengthy comment.  That's just how
I
> would do it, but feel free to do it this way if you prefer.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode400
net/spdy/spdy_network_transaction_unittest.cc:400: // session.  Once we have the
sesion, we verify that the streams are
On 2010/08/22 16:20:50, willchan wrote:
> session

Done.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode402
net/spdy/spdy_network_transaction_unittest.cc:402: GURL url =
helper.request().url;
On 2010/08/22 16:20:50, willchan wrote:
> const GURL&

Done.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode410
net/spdy/spdy_network_transaction_unittest.cc:410: scoped_refptr<SpdySession>
spdy_session(pool->Get(pair, session, log));
On 2010/08/22 17:40:45, erikchen1 wrote:
> Doesn't pool->Get create a session, if it doesn't already exist? you probably
> want to ASSERT_TRUE(pool->HasSession(pair)).

See line 409 :-)

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2178
net/spdy/spdy_network_transaction_unittest.cc:2178: 'p', 'u', 's', 'h', 'e', 'd'
                                // "hello"
On 2010/08/22 16:20:50, willchan wrote:
> update comment

Done.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2186
net/spdy/spdy_network_transaction_unittest.cc:2186: scoped_ptr<spdy::SpdyFrame>
resp(ConstructSpdyGetSynReply(NULL, 0, 1));
On 2010/08/22 16:20:50, willchan wrote:
> Meta-comment: these variable names suck.  resp and rep?  It's confusing.  I
> think of resp == response and rep == reply.  But It's not obvious what the
> responses/replies are to.  Nothing you have to fix in this changelist, but I
> thought I'd mention it.

Done.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2191
net/spdy/spdy_network_transaction_unittest.cc:2191: CreateMockRead(*body.get(),
4, false),
On 2010/08/22 16:20:50, willchan wrote:
> Is this a bugfix?  Is it because we don't have a FIN on resp?

The test didn't use to really finish the streams - it allowed the session to be
torn down while the streams were still active.  This was okay, but I needed to
check that our pushed streams (which had completed) were fully sent.  So, I took
the approach of cleaning up the tests so that they fully send all streams. 
Then, when I tear down the stream, I can verify that there are no unfinished
streams...  if there are, it is now a bug which the test can catch.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2216
net/spdy/spdy_network_transaction_unittest.cc:2216: 'p', 'u', 's', 'h', 'e', 'd'
                                // "hello"
On 2010/08/22 16:20:50, willchan wrote:
> update comment

Done.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2264
net/spdy/spdy_network_transaction_unittest.cc:2264: 'p', 'u', 's', 'h', 'e', 'd'
                                // "hello"
On 2010/08/22 16:20:50, willchan wrote:
> update comment

Done.

http://codereview.chromium.org/3108038/diff/14001/15002#newcode2291
net/spdy/spdy_network_transaction_unittest.cc:2291: MockRead(true,
ERR_IO_PENDING, 9)  // Force a pause.
On 2010/08/22 17:40:45, erikchen1 wrote:
> s/9/10

Done.

http://codereview.chromium.org/3108038/diff/14001/15003
File net/spdy/spdy_session.cc (left):

http://codereview.chromium.org/3108038/diff/14001/15003#oldcode866
net/spdy/spdy_session.cc:866: }
On 2010/08/22 17:40:45, erikchen1 wrote:
> You may want to consider the edge case where a server pushes a stream, and
then
> sends a RST_STREAM for that stream. Shouldn't that remove the pushed stream as
> well? Perhaps that can be moved to a separate function.

Good point. Test case "ServerPushServerAborted" added.

http://codereview.chromium.org/3108038/diff/14001/15003
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15003#newcode201
net/spdy/spdy_session.cc:201: RecordHistograms();
On 2010/08/22 16:20:50, willchan wrote:
> I think that at the end of SpdySession(), we should DCHECK all the stream
lists
> to make sure they're empty.  CloseAllStreams() should have cleared them all
out
> by this point.

Done.

http://codereview.chromium.org/3108038/diff/14001/15003#newcode781
net/spdy/spdy_session.cc:781: if (!unclaimed_pushed_streams_.empty()) {
On 2010/08/22 16:20:50, willchan wrote:
> Should we also do unclaimed_pushed_streams_.clear(); here?  Otherwise, we're
not
> closing all streams.

Done.

http://codereview.chromium.org/3108038/diff/14001/15004
File net/spdy/spdy_session.h (right):

http://codereview.chromium.org/3108038/diff/14001/15004#newcode167
net/spdy/spdy_session.h:167: size_t num_active_streams() { return
active_streams_.size(); }
On 2010/08/22 16:20:50, willchan wrote:
> These new accessors should be const.

Done.

http://codereview.chromium.org/3108038/diff/14001/15005
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15005#newcode189
net/spdy/spdy_stream.cc:189: scoped_refptr<SpdyStream> self(this);
On 2010/08/22 16:20:50, willchan wrote:
> Can we avoid this self-reference?  It looks like you do it because
CloseStream()
> could remove the last reference and delete the object, which would cause
badness
> in UpdateHistograms().  Can we move UpdateHistograms into the destructor?  Or
> does that cause problems since we might be updating the histograms too late? 
> It'd also remove the need to have the boolean that checks to make sure we
don't
> update the histograms more than once per stream.

Done.

http://codereview.chromium.org/3108038/diff/14001/15005#newcode189
net/spdy/spdy_stream.cc:189: scoped_refptr<SpdyStream> self(this);
On 2010/08/22 17:40:45, erikchen1 wrote:
> I remember moving UpdateHistograms() into the destructor in some CL that got
> lost in the annals of time. You have to add a bool that gets updated when we
> start recording histograms, because calling UpdateHistograms() before calling
> StartHisotgrams(), or whatever that call is, will result in a CHECK. 
> I don't _think_ there was a problem with calling UpdateHistograms too late.


> 

Looks like UpdateHistograms already checks that the start timers have been set
properly, and if they haven't, it bails.

> 
> On 2010/08/22 16:20:50, willchan wrote:
> > Can we avoid this self-reference?  It looks like you do it because
> CloseStream()
> > could remove the last reference and delete the object, which would cause
> badness
> > in UpdateHistograms().  Can we move UpdateHistograms into the destructor? 
Or
> > does that cause problems since we might be updating the histograms too late?

> > It'd also remove the need to have the boolean that checks to make sure we
> don't
> > update the histograms more than once per stream.
> 
>

http://codereview.chromium.org/3108038/diff/14001/15005#newcode190
net/spdy/spdy_stream.cc:190: session_->CloseStream(stream_id_, net::OK);
Erik's comment is correct, we will call OnClose.  But, it's kind of irrelevant. 
In this case, we're buffering up data precisely because there *is no callback*
:-)  (note that line 179 checks for !delegate || continue_buffering_data).

On 2010/08/22 16:20:50, willchan wrote:
> Now that we're closing the stream here, which removes it from the active
> streams, a subsequent CloseAllStreams() won't call OnClose() on this stream,
> since it's only in the unclaimed push streams list.  Perhaps you need have
> DeleteStream() or CloseAllStreams() do this.  Not calling OnClose() is
currently
> benign, but seems "wrong", since you'd expect that CloseAllStreams() would
call
> OnClose() on all streams.  I'm afraid that it's possible for a future refactor
> to introduce a bug here.

[willchan no longer on Chromium, 2010-08-22 23:35:38]

lgtm, thanks for the fix!

http://codereview.chromium.org/3108038/diff/14001/15005
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/3108038/diff/14001/15005#newcode190
net/spdy/spdy_stream.cc:190: session_->CloseStream(stream_id_, net::OK);
Woops, I missed that.  And yeah, I know it's currently irrelevant/benign.  I was
just worried about future refactors.

On 2010/08/22 21:52:29, Mike Belshe wrote:
> Erik's comment is correct, we will call OnClose.  But, it's kind of
irrelevant. 
> In this case, we're buffering up data precisely because there *is no callback*
> :-)  (note that line 179 checks for !delegate || continue_buffering_data).
> 
> On 2010/08/22 16:20:50, willchan wrote:
> > Now that we're closing the stream here, which removes it from the active
> > streams, a subsequent CloseAllStreams() won't call OnClose() on this stream,
> > since it's only in the unclaimed push streams list.  Perhaps you need have
> > DeleteStream() or CloseAllStreams() do this.  Not calling OnClose() is
> currently
> > benign, but seems "wrong", since you'd expect that CloseAllStreams() would
> call
> > OnClose() on all streams.  I'm afraid that it's possible for a future
refactor
> > to introduce a bug here.
> 
>

http://codereview.chromium.org/3108038/diff/24001/23003
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3108038/diff/24001/23003#newcode248
net/spdy/spdy_network_transaction_unittest.cc:248: HttpNetworkTransaction*
trans() const { return trans_.get(); }
I don't think trans() nor session() should be const, since I view this object as
conceptually owning those objects, so a const |this| should not give non-const
access to those objects.

http://codereview.chromium.org/3108038/diff/24001/23003#newcode358
net/spdy/spdy_network_transaction_unittest.cc:358:
EXPECT_TRUE(spdy_session.get() != NULL);
ASSERT_TRUE, since the following lines will crash if it's not true.

http://codereview.chromium.org/3108038/diff/24001/23006
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/3108038/diff/24001/23006#newcode190
net/spdy/spdy_stream.cc:190: scoped_refptr<SpdyStream> self(this);
Do you need the self-reference still?  I think we just need to add a comment
after CloseStream() saying it might be deleted after this point.

http://codereview.chromium.org/3108038/diff/24001/23006#newcode209
net/spdy/spdy_stream.cc:209: scoped_refptr<SpdyStream> self(this);
Ditto on whether we need the self-reference.

[Mike Belshe, 2010-08-23 03:23:07]

Thanks for the quick turnaround on this!

http://codereview.chromium.org/3108038/diff/24001/23003
File net/spdy/spdy_network_transaction_unittest.cc (right):

http://codereview.chromium.org/3108038/diff/24001/23003#newcode248
net/spdy/spdy_network_transaction_unittest.cc:248: HttpNetworkTransaction*
trans() const { return trans_.get(); }
On 2010/08/22 23:35:39, willchan wrote:
> I don't think trans() nor session() should be const, since I view this object
as
> conceptually owning those objects, so a const |this| should not give non-const
> access to those objects.

I certainly agree on the HttpNetworkTransaction - sorry about that.  Fort he
Session, I'd prefer const since we generally use it for just grabbing const
members.

http://codereview.chromium.org/3108038/diff/24001/23003#newcode358
net/spdy/spdy_network_transaction_unittest.cc:358:
EXPECT_TRUE(spdy_session.get() != NULL);
On 2010/08/22 23:35:39, willchan wrote:
> ASSERT_TRUE, since the following lines will crash if it's not true.

Done.

http://codereview.chromium.org/3108038/diff/24001/23006
File net/spdy/spdy_stream.cc (right):

http://codereview.chromium.org/3108038/diff/24001/23006#newcode190
net/spdy/spdy_stream.cc:190: scoped_refptr<SpdyStream> self(this);
On 2010/08/22 23:35:39, willchan wrote:
> Do you need the self-reference still?  I think we just need to add a comment
> after CloseStream() saying it might be deleted after this point.

Done.

http://codereview.chromium.org/3108038/diff/24001/23006#newcode209
net/spdy/spdy_stream.cc:209: scoped_refptr<SpdyStream> self(this);
On 2010/08/22 23:35:39, willchan wrote:
> Ditto on whether we need the self-reference.

Done.

[mbelshe, 2010-08-23 04:08:43]

Thanks for taking the time to comment, Erik.

On Sun, Aug 22, 2010 at 10:40 AM, <erikchen@chromium.org> wrote:

> Hey Mike.
> So the reason I didn't want to remove the pushed stream from
> active_streams_
> until the 0 data read gets reread is that would make OnClose get called at
> the
> right time. Under the current api, OnClose gets called only and exactly
> when the
> stream is removed from active_streams_. I guess the current tradeoff is
> between
> accuracy of the abstraction (should active_streams_ contain a pushed stream
> that
> has theoretically been told to finish by the server) and centralization of
> code.
>

OK - I'm concerned about leaving it as "active", because frames arriving for
the stream will be dealt with differently based on which state it is in.


> Your call.
> Another thought. Not for this CL, but an important fix for SPDY tests I
> never
> added. Every test that uses the spdy_session_pool should check at the end
> that
> the pool is empty. Otherwise it is possible that a spdysession will leak
> and we
> won't know about it (unless a future test calls OnIPAddressChanged, and
> gets a
> weird error)


Agree


>
>
> On 2010/08/22 16:20:49, willchan wrote:
>
>> Everything works and LGTM as is, but I've made a number of suggestions
>> which I
>> think we should probably implement, but don't necessarily have to be done
>> in
>> this changelist.
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002
>> File net/spdy/spdy_network_transaction_unittest.cc (right):
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002#newcode337
>> net/spdy/spdy_network_transaction_unittest.cc:337: std::string
>> result1(buf->data(), rv);
>> This result1 is unnecessary.  Just do result->append(buf->data(), rv);
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002#newcode398
>> net/spdy/spdy_network_transaction_unittest.cc:398: {
>> Nit: Is there a reason you need to scope this?  Is it to make it clear
>> what
>>
> the
>
>> code is doing?  It'd probably be better to write a GetSpdySessionForURL()
>> function or something, rather than writing a lengthy comment.  That's just
>> how
>>
> I
>
>> would do it, but feel free to do it this way if you prefer.
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002#newcode400
>> net/spdy/spdy_network_transaction_unittest.cc:400: // session.  Once we
>> have
>>
> the
>
>> sesion, we verify that the streams are
>> session
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002#newcode402
>> net/spdy/spdy_network_transaction_unittest.cc:402: GURL url =
>> helper.request().url;
>> const GURL&
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002#newcode2178
>> net/spdy/spdy_network_transaction_unittest.cc:2178: 'p', 'u', 's', 'h',
>> 'e',
>>
> 'd'
>
>>                                 // "hello"
>> update comment
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002#newcode2186
>> net/spdy/spdy_network_transaction_unittest.cc:2186:
>>
> scoped_ptr<spdy::SpdyFrame>
>
>> resp(ConstructSpdyGetSynReply(NULL, 0, 1));
>> Meta-comment: these variable names suck.  resp and rep?  It's confusing.
>>  I
>> think of resp == response and rep == reply.  But It's not obvious what the
>> responses/replies are to.  Nothing you have to fix in this changelist, but
>> I
>> thought I'd mention it.
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002#newcode2191
>> net/spdy/spdy_network_transaction_unittest.cc:2191:
>>
> CreateMockRead(*body.get(),
>
>> 4, false),
>> Is this a bugfix?  Is it because we don't have a FIN on resp?
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002#newcode2216
>> net/spdy/spdy_network_transaction_unittest.cc:2216: 'p', 'u', 's', 'h',
>> 'e',
>>
> 'd'
>
>>                                 // "hello"
>> update comment
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15002#newcode2264
>> net/spdy/spdy_network_transaction_unittest.cc:2264: 'p', 'u', 's', 'h',
>> 'e',
>>
> 'd'
>
>>                                 // "hello"
>> update comment
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15003
>> File net/spdy/spdy_session.cc (right):
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15003#newcode201
>> net/spdy/spdy_session.cc:201: RecordHistograms();
>> I think that at the end of SpdySession(), we should DCHECK all the stream
>>
> lists
>
>> to make sure they're empty.  CloseAllStreams() should have cleared them
>> all
>>
> out
>
>> by this point.
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15003#newcode781
>> net/spdy/spdy_session.cc:781: if (!unclaimed_pushed_streams_.empty()) {
>> Should we also do unclaimed_pushed_streams_.clear(); here?  Otherwise,
>> we're
>>
> not
>
>> closing all streams.
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15004
>> File net/spdy/spdy_session.h (right):
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15004#newcode167
>> net/spdy/spdy_session.h:167: size_t num_active_streams() { return
>> active_streams_.size(); }
>> These new accessors should be const.
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15005
>> File net/spdy/spdy_stream.cc (right):
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15005#newcode189
>> net/spdy/spdy_stream.cc:189: scoped_refptr<SpdyStream> self(this);
>> Can we avoid this self-reference?  It looks like you do it because
>>
> CloseStream()
>
>> could remove the last reference and delete the object, which would cause
>>
> badness
>
>> in UpdateHistograms().  Can we move UpdateHistograms into the destructor?
>>  Or
>> does that cause problems since we might be updating the histograms too
>> late?
>> It'd also remove the need to have the boolean that checks to make sure we
>>
> don't
>
>> update the histograms more than once per stream.
>>
>
>  http://codereview.chromium.org/3108038/diff/14001/15005#newcode190
>> net/spdy/spdy_stream.cc:190: session_->CloseStream(stream_id_, net::OK);
>> Now that we're closing the stream here, which removes it from the active
>> streams, a subsequent CloseAllStreams() won't call OnClose() on this
>> stream,
>> since it's only in the unclaimed push streams list.  Perhaps you need have
>> DeleteStream() or CloseAllStreams() do this.  Not calling OnClose() is
>>
> currently
>
>> benign, but seems "wrong", since you'd expect that CloseAllStreams() would
>>
> call
>
>> OnClose() on all streams.  I'm afraid that it's possible for a future
>> refactor
>> to introduce a bug here.
>>
>
>
>
> http://codereview.chromium.org/3108038/show
>