SPDY - chunked upload - speech recognition doesn't work with SPDY/3

SPDY - chunked upload - cleanup of spdy_http_stream to read the data
in SendBody instead of SendBodyComplete.

SpdyHttpStream inherits from ChunkCallback (SpdyStream doesn't inherit
from ChunkCallback). OnChunkAvailable is handled by SpdyHttpStream.

In unit tests, added new tests (similar to http_stream_parser_unittest
for chunked uploads), to delay sending of chunked data.

Also added sending of window_update during chunk uploads. Verified
that these tests fail with the old code and run ok with the new changes.

This change also fixed chunked uploads hanging (or stalling) for
speech recgonition.

Many many thanks to sleevi for suggesting the way to clean up
the SpdyHttpStream's SendBody and SendBodyComplete code.

R=rch@chromium.org, rsleevi@chromium.org
TEST=network unit tests
BUG=113107, 136044
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146255

[ramant (doing other things), 2012-06-30 23:10:29]

Hi willchan and rch,
  Could you take a look this change to handle WINDOW_UPDATE messages from server
when we are doing chunked uploads.

thanks,
raman

[Ryan Sleevi, 2012-07-02 21:52:46]

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_http_...
File net/spdy/spdy_http_stream_spdy3_unittest.cc (right):

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:225: TestCompletionCallback
callback0;
nit: You can re-use existing TestCompletionCallbacks, which may make more sense
here, rather than requiring callback0.

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:252:
http_stream->InitializeStream(&request, net_log, CompletionCallback()));
nit: Would this be more readable as

ASSERT_EQ(OK,
          http_stream->InitializeStream(&request, net_log,
                                        CompletionCallback()));

Same number of lines (3), but a little more readable I think. It also parallels
lines 238/239 and 228/229

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:265: EXPECT_EQ(ERR_IO_PENDING,
http_stream->SendRequest(
whitespace nit:

EXPECT_EQ(ERR_IO_PENDING,
          http_stream->SendRequest(...) )?

Not critical at all.

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:276: // Finalize writing the last
chunk, which will enqueue the trailer.
Does |trailer| make sense here? In the case of HTTP chunked uploads, the meaning
of trailer is defined as part of the Transfer-Encoding. Does SPDY also have a
concept of "trailer" when doing chunked uploads? Would this be better with more
context?

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:281: data->RunFor(2);
nit: Add a comment explaining what the two events are

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:285:
ASSERT_TRUE(response.headers.get() != NULL);
I thought the preference was instead

ASSERT_TRUE(response.headers.get())

eg: no explicit NULL check

(This is the same as using "if (foo)" in conditionals). I'm not sure if the !=
NULL pattern is more prevalent in SPDY code, but I see the "if (foo)" as more
prevalent in general Chromium code)

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_strea...
net/spdy/spdy_stream.h:64: virtual int OnSendBodyComplete(int status, bool* eof)
= 0;
In the case of SPDY, which already (seemingly) supports streaming bodies, it's
not clear how this method is different from OnSendChunkedBody.

Just from reading the comment on OnSendBodyComplete, it seems like no new
delegate methods should be added, and instead the delegate should set eof to
false if IsRequestBodyChunked.

The actual SpdyStream should stay in STATE_SEND_BODY until OnSendBodyComplete
successfully indicates EOF, at which point it should then treat the full body as
written.

It sounds like the bug would still present itself if a delegate set |eof| to
false, since that would involve a transition in the stream from STATE_SEND_BODY
to STATE_SEND_BODY_COMPLETE back to STATE_SEND_BODY

[Ryan Hamilton, 2012-07-02 22:34:32]

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_http_...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:181: TEST_F(SpdyHttpStreamSpdy2Test,
DelayedSendChunkedPost) {
Does this test fail when run w/o your changes to the non-test classes?

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

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_strea...
net/spdy/spdy_stream.cc:179: return;
This still feels totally wrong to me.  I do not think it makes sense to silently
ignore this condition.  Either it should be an *error* to get WINDOW_UPDATE when
we don't expect it, or we should handle them.

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_strea...
net/spdy/spdy_stream.h:64: virtual int OnSendBodyComplete(int status, bool* eof)
= 0;
On 2012/07/02 21:52:47, Ryan Sleevi wrote:
> In the case of SPDY, which already (seemingly) supports streaming bodies, it's
> not clear how this method is different from OnSendChunkedBody.
> 
> Just from reading the comment on OnSendBodyComplete, it seems like no new
> delegate methods should be added, and instead the delegate should set eof to
> false if IsRequestBodyChunked.
> 
> The actual SpdyStream should stay in STATE_SEND_BODY until OnSendBodyComplete
> successfully indicates EOF, at which point it should then treat the full body
as
> written.
> 
> It sounds like the bug would still present itself if a delegate set |eof| to
> false, since that would involve a transition in the stream from
STATE_SEND_BODY
> to STATE_SEND_BODY_COMPLETE back to STATE_SEND_BODY

I agree with sleevi here.

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_strea...
File net/spdy/spdy_stream_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10689034/diff/5003/net/spdy/spdy_strea...
net/spdy/spdy_stream_spdy2_unittest.cc:84: virtual bool IsRequestBodyChunked() {
return false; }
Should we add a test where this returns true?

[Ryan Sleevi, 2012-07-04 00:52:49]

I haven't closely looked at the tests, but I think the fix is spot on, in that
it places the knowledge of when to write with the class that is responsible for
doing the write.

Some notes below, nothing major:

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:282: waiting_for_chunk_ = true;
Note that this does mean you can get in a weird state where

SendData
  request_body_stream_->Read == ERR_IO_PENDING
    (waiting_for_chunk_ = true, return ERR_IO_PENDING)

OnChunkAvailable
  waiting_for_chunk_ = false
  SendData

SendData
  request_body_stream_ == ERR_IO_PENDING

The second ERR_IO_PENDING shouldn't ever happen, I believe. It'd be nice to
DCHECK appropriately.

One way to do that would be do not reset waiting_for_chunk_ in OnChunkAvailable,
and instead reset it here in SendData

const int bytes_read = ...
DCHECK(!waiting_for_chunk_ || bytes_read != ERR_IO_PENDING)

if (request_body_stream_-> ...) {
  waiting_for_chunk_ = true;
  return ERR_IO_PENDING;
}

waiting_for_chunk_ = false;

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:292: DCHECK_GE(request_body_buf_->BytesRemaining(),
0);
This check is probably overkill, imo. It's not testing the correctness of this
code, it's checking the correctness of DrainableIOBuffer.

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:432: if (waiting_for_chunk_) {
nit: You can handle this as a short circuit, to reduce a level of
indentation/scope.

if (!waiting_for_chunk_)
  return;

DCHECK(...)
SendData()

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:35: public ChunkCallback {
iwyu: You should include "net/base/upload_data.h" to use ChunkCallback.

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:102: // spdy stream's WriteStreamData. In case of
chunked uploads, if data is not
nit: spdy stream's WriteStreamData -> SpdyStream::WriteStreamData

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:104: // chunked data by setting |waiting_for_chunk_|
to true.
nit: Suggested rewording:

In the case of chunked uploads, if data is not available, returns
ERR_IO_PENDING, and the write will be completed at a later time.

I would not mention too many details about /how/ it does what it does, just what
it will do.

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:159: // Is the spdy stream waiting for chunked data.
nit: True if the connection is stalled waiting for a upload data chunk.

[ramant (doing other things), 2012-07-04 21:04:32]

Hi rch@ and sleevi@,
  We have rewritten this code (not much of the original code is remaining except
for the unit tests). Would appreciate if you could take a fresh look at it.

thanks
raman

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:181: TEST_F(SpdyHttpStreamSpdy2Test,
DelayedSendChunkedPost) {
On 2012/07/02 22:34:32, Ryan Hamilton wrote:
> Does this test fail when run w/o your changes to the non-test classes?

It doesn't fail. IMO, it is just new test.

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:225: TestCompletionCallback
callback0;
On 2012/07/02 21:52:47, Ryan Sleevi wrote:
> nit: You can re-use existing TestCompletionCallbacks, which may make more
sense
> here, rather than requiring callback0.

Done.

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:252:
http_stream->InitializeStream(&request, net_log, CompletionCallback()));
On 2012/07/02 21:52:47, Ryan Sleevi wrote:
> nit: Would this be more readable as
> 
> ASSERT_EQ(OK,
>           http_stream->InitializeStream(&request, net_log,
>                                         CompletionCallback()));
> 
> Same number of lines (3), but a little more readable I think. It also
parallels
> lines 238/239 and 228/229

Done.

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:265: EXPECT_EQ(ERR_IO_PENDING,
http_stream->SendRequest(
On 2012/07/02 21:52:47, Ryan Sleevi wrote:
> whitespace nit:
> 
> EXPECT_EQ(ERR_IO_PENDING,
>           http_stream->SendRequest(...) )?
> 
> Not critical at all.

Done.

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:276: // Finalize writing the last
chunk, which will enqueue the trailer.
On 2012/07/02 21:52:47, Ryan Sleevi wrote:
> Does |trailer| make sense here? In the case of HTTP chunked uploads, the
meaning
> of trailer is defined as part of the Transfer-Encoding. Does SPDY also have a
> concept of "trailer" when doing chunked uploads? Would this be better with
more
> context?

You are right. "trailer" doesn't make sense.
Done.

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:281: data->RunFor(2);
On 2012/07/02 21:52:47, Ryan Sleevi wrote:
> nit: Add a comment explaining what the two events are

Done.

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:285:
ASSERT_TRUE(response.headers.get() != NULL);
On 2012/07/02 21:52:47, Ryan Sleevi wrote:
> I thought the preference was instead
> 
> ASSERT_TRUE(response.headers.get())
> 
> eg: no explicit NULL check
> 
> (This is the same as using "if (foo)" in conditionals). I'm not sure if the !=
> NULL pattern is more prevalent in SPDY code, but I see the "if (foo)" as more
> prevalent in general Chromium code)

Done.

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

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_stream.cc#new...
net/spdy/spdy_stream.cc:179: return;
On 2012/07/02 22:34:32, Ryan Hamilton wrote:
> This still feels totally wrong to me.  I do not think it makes sense to
silently
> ignore this condition.  Either it should be an *error* to get WINDOW_UPDATE
when
> we don't expect it, or we should handle them.

Done.

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_stream.h
File net/spdy/spdy_stream.h (right):

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_stream.h#newc...
net/spdy/spdy_stream.h:64: virtual int OnSendBodyComplete(int status, bool* eof)
= 0;
On 2012/07/02 21:52:47, Ryan Sleevi wrote:
> In the case of SPDY, which already (seemingly) supports streaming bodies, it's
> not clear how this method is different from OnSendChunkedBody.
> 
> Just from reading the comment on OnSendBodyComplete, it seems like no new
> delegate methods should be added, and instead the delegate should set eof to
> false if IsRequestBodyChunked.
> 
> The actual SpdyStream should stay in STATE_SEND_BODY until OnSendBodyComplete
> successfully indicates EOF, at which point it should then treat the full body
as
> written.
> 
> It sounds like the bug would still present itself if a delegate set |eof| to
> false, since that would involve a transition in the stream from
STATE_SEND_BODY
> to STATE_SEND_BODY_COMPLETE back to STATE_SEND_BODY

sleevi and I had walked through the code and verified that there is no bug. But
the code in spdy_http_stream.cc how it handles the first upload data is slightly
complicated (cases to prove to ourselves that there is no loop or data is not
duplicated). Will file a separate bug to clean up the code.

Done.

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_stream_spdy2_...
File net/spdy/spdy_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/5003/net/spdy/spdy_stream_spdy2_...
net/spdy/spdy_stream_spdy2_unittest.cc:84: virtual bool IsRequestBodyChunked() {
return false; }
On 2012/07/02 22:34:32, Ryan Hamilton wrote:
> Should we add a test where this returns true?

Deleted this code (because it is not needed).

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:282: waiting_for_chunk_ = true;
On 2012/07/04 00:52:49, Ryan Sleevi wrote:
> Note that this does mean you can get in a weird state where
> 
> SendData
>   request_body_stream_->Read == ERR_IO_PENDING
>     (waiting_for_chunk_ = true, return ERR_IO_PENDING)
> 
> OnChunkAvailable
>   waiting_for_chunk_ = false
>   SendData
> 
> SendData
>   request_body_stream_ == ERR_IO_PENDING
> 
> The second ERR_IO_PENDING shouldn't ever happen, I believe. It'd be nice to
> DCHECK appropriately.
> 
> One way to do that would be do not reset waiting_for_chunk_ in
OnChunkAvailable,
> and instead reset it here in SendData
> 
> const int bytes_read = ...
> DCHECK(!waiting_for_chunk_ || bytes_read != ERR_IO_PENDING)
> 
> if (request_body_stream_-> ...) {
>   waiting_for_chunk_ = true;
>   return ERR_IO_PENDING;
> }
> 
> waiting_for_chunk_ = false;

Done.

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:292: DCHECK_GE(request_body_buf_->BytesRemaining(),
0);
On 2012/07/04 00:52:49, Ryan Sleevi wrote:
> This check is probably overkill, imo. It's not testing the correctness of this
> code, it's checking the correctness of DrainableIOBuffer.

Done.

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:432: if (waiting_for_chunk_) {
On 2012/07/04 00:52:49, Ryan Sleevi wrote:
> nit: You can handle this as a short circuit, to reduce a level of
> indentation/scope.
> 
> if (!waiting_for_chunk_)
>   return;
> 
> DCHECK(...)
> SendData()

Done.

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:35: public ChunkCallback {
On 2012/07/04 00:52:49, Ryan Sleevi wrote:
> iwyu: You should include "net/base/upload_data.h" to use ChunkCallback.

Done.

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:102: // spdy stream's WriteStreamData. In case of
chunked uploads, if data is not
On 2012/07/04 00:52:49, Ryan Sleevi wrote:
> nit: spdy stream's WriteStreamData -> SpdyStream::WriteStreamData

Done.

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:104: // chunked data by setting |waiting_for_chunk_|
to true.
On 2012/07/04 00:52:49, Ryan Sleevi wrote:
> nit: Suggested rewording:
> 
> In the case of chunked uploads, if data is not available, returns
> ERR_IO_PENDING, and the write will be completed at a later time.
> 
> I would not mention too many details about /how/ it does what it does, just
what
> it will do.

Done.

http://codereview.chromium.org/10689034/diff/9006/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:159: // Is the spdy stream waiting for chunked data.
On 2012/07/04 00:52:49, Ryan Sleevi wrote:
> nit: True if the connection is stalled waiting for a upload data chunk.

Done.

[Ryan Sleevi, 2012-07-05 06:52:06]

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream.cc
File net/spdy/spdy_http_stream.cc (right):

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:245:
DCHECK_EQ(static_cast<HttpResponseInfo*>(NULL), response_info_);
style nit: this should be

} else {
  DCHECK_EQ(...)
}

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...
- "However, if one part" ...

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream.c...
net/spdy/spdy_http_stream.cc:424:
request_body_stream_->set_chunk_callback(NULL);
Does Cancel also need to be updated?

Not sure the lifetime semantics there, but it is resetting stream_

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:89: // ChunkCallback methods.
nit: ChunkCallback implementation.

Matches lines 49/78

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:160: // Set to True if the connection is stalled
waiting for a upload data chunk.
nit: True -> true, and it seems like "set to" should be dropped, a upload -> an
upload.

However, looking at the other comments for bools, this may make more sense as a
question "Is this/has this", although grammatically I think those are also a bit
weird.

(Consistent with this file)
// Is the connection stalled waiting for an upload data chunk.

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:182:
UploadDataStream::set_merge_chunks(false);
You appear to append chunks sequentially, at least when reading this test. Every
chunk appending (258, 275, 281) have matching data->RunFor(1) calls, so it
doesn't seem clear how/when you could ever merge chunks, or why merging chunks
would be bad for the correctness of this test.

Is this necessary? If so, it would be good to comment why - and why this test
does not reset any values upon completion (it would seem to have global
side-effects).

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:284: data->RunFor(3);
As mentioned in-person, I think it would be good to also make sure that
appending two chunks back to back, without any data->RunFor() calls in between,
would be good. You want to make sure that multiple calls to OnChunkAvailable
still result in the correct behaviour within the SpdyHttpStream state machine.

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:293: EXPECT_EQ(kUploadData,
std::string(buf1->data(), kUploadDataSize));
BUG?: It seems like these tests should be checking the result of
ReadResponseBody() and making sure that callback was called.

This is "safe", only because |buf1| is pre-allocated to be kUploadDataSize (line
291), so the copy to a string on line 293 won't read past the end.

However, since this is subtle, especially when combined with the fact that
you're not checking that callback.callback() is called before line 295, it makes
me wonder whether or not the test will fail gracefully if ReadResponseBody ever
returned a different size.

Same for the further reads below

[Ryan Sleevi, 2012-07-06 20:20:03]

Hi Raman,

I removed sleevi@c.o from the reviewers - since it's rsleevi@.  I looked over
the diff from patchset 7 to 9.

To be clear, I think you're testing the right functionality, but I think the
test still has some ingrained assumptions about the behaviours of the underlying
SpdyStream/SpdyHttpStream. These assumptions should either be explicitly part of
the test, or the test should be worked around to not assume anything.

I've highlighted below the two places where I think there are assumptions that,
if the underlying implementation changed, wouldn't hold up, and a suggestion for
how to address these. Also, one nit below about having the test methods kept
purely as private methods.

Hopefully this will be good to go in another cycle. Thanks for working on this!

http://codereview.chromium.org/10689034/diff/2007/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/10689034/diff/2007/net/base/upload_data_stream...
net/base/upload_data_stream.h:68: // This method is provided only to be used by
unit tests.
nit: This method is -> These methods are

However, I'd suggest you make these private and either friend the
forward-declared unit test fixtures directly (SpdyHttpStreamSpdy[2/3]Test and
SpdyNetworkTransactionSpdy[2/3]test). Since individual tests derive from the
base class, you would need to expose a protected method on the test fixtures
that called these private methods.

Still, better than exposing these publicly, I think.

http://codereview.chromium.org/10689034/diff/2007/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/2007/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:287:
ASSERT_TRUE(callback.have_result());
I would add an

ASSERT_FALSE(callback.have_result()) on lines 280 and between 282/283

The point is that callback.have_result() should only be true once the
UploadDataStream has indicated EOF. Until then, it should remain false -
correct? If so, lets explicitly test that.

http://codereview.chromium.org/10689034/diff/2007/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:296: data->RunFor(4);
It's not clear to me how this is better.

In particular, if for some reason all four reads are NOT consumed, this will run
off into infinity (or a test timeout), which is a similar-but-different BUG?
then the one I had mentioned in the previous comment.

My thought was that (in the old patch), you'd simply add something between lines
300 - 301
int rv = http_stream->ReadResponseBody(...)
ASSERT_EQ(kUploadDataSize, rv);
EXPECT_EQ(...)

The fact is, ReadResponseBody takes a net::CompletionCallback, therefore it's
valid for it to return ERR_IO_PENDING. This test assumes that the only way
ERR_IO_PENDING can be returned is if the network IO is slow - but I don't think
that's at all guaranteed by the interface.

Because this test is not setup to handle ERR_IO_PENDING, you should explicitly
ASSERT you only ever get your expected result.

I also think your RunFor(1) + those ASSERTs are safer than RunFor(4), since the
RunFor(4) assumes (again, not guaranteed by the interface) that the SpdyStream
will consume all data available to it from the network. I don't know if that's
guaranteed, since there is a degree of internal buffering that happens.

[Ryan Sleevi, 2012-07-06 21:04:45]

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/1030/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:295: data->RunFor(1);
I just realized there's another bug here in the code:

Your pairing of RunFor() and ReadResponseBody calls are out of order.

Instead of doing ReadResponseBody -> RunFor, you're doing RunFor ->
ReadResponseBody. This is (implicitly) relying on behaviour that the underlying
socket is greedy and will read as much as possible.

Instead, it should be

int rv = http_stream->ReadResponseBody(...)
ASSERT_EQ(ERR_IO_PENDING, rv);
data->RunFor(1);
ASSERT_TRUE(callback.have_result());
EXPECT_EQ(kUploadDataSize, callback.WaitForResult());

rv = ReadResponseBody(buf2, ...)
ASSERT_EQ(ERR_IO_PENDING, rv);
data->RunFor(1);

etc

The point being that you shouldn't pump the underlying socket until you have
already issued the read request.

[Yes, I'm aware of the fact that protocol semantics such as PING and
WINDOW_UPDATE at the Session layer imply that the underlying transport will
always have a standing read, even while only writing, but it would seem that in
the higher level code, such behaviour should be an implementation detail and not
part of the tests' assumptions]

[ramant (doing other things), 2012-07-06 22:55:42]

Hi Ryan Sleevi,
  Many many thanks for your comments. Spdy is slightly different with callback
(Callback has_result was always true). Would appreciate your comments.

thanks,
raman

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
File net/spdy/spdy_http_stream.cc (right):

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
net/spdy/spdy_http_stream.cc:245:
DCHECK_EQ(static_cast<HttpResponseInfo*>(NULL), response_info_);
On 2012/07/05 06:52:06, Ryan Sleevi wrote:
> style nit: this should be
> 
> } else {
>   DCHECK_EQ(...)
> }
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Condit...
> - "However, if one part" ...

Done.

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
net/spdy/spdy_http_stream.cc:424:
request_body_stream_->set_chunk_callback(NULL);
On 2012/07/05 06:52:06, Ryan Sleevi wrote:
> Does Cancel also need to be updated?
> 
> Not sure the lifetime semantics there, but it is resetting stream_

No need. Cancel calls reset spdy stream, which deletes the stream (which calls
OnClose and which calls SpdyHttpStream's OnClose).

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
File net/spdy/spdy_http_stream.h (right):

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
net/spdy/spdy_http_stream.h:89: // ChunkCallback methods.
On 2012/07/05 06:52:06, Ryan Sleevi wrote:
> nit: ChunkCallback implementation.
> 
> Matches lines 49/78

Done.

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
net/spdy/spdy_http_stream.h:160: // Set to True if the connection is stalled
waiting for a upload data chunk.
On 2012/07/05 06:52:06, Ryan Sleevi wrote:
> nit: True -> true, and it seems like "set to" should be dropped, a upload ->
an
> upload.
> 
> However, looking at the other comments for bools, this may make more sense as
a
> question "Is this/has this", although grammatically I think those are also a
bit
> weird.
> 
> (Consistent with this file)
> // Is the connection stalled waiting for an upload data chunk.

Done.

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:182:
UploadDataStream::set_merge_chunks(false);
On 2012/07/05 06:52:06, Ryan Sleevi wrote:
> You appear to append chunks sequentially, at least when reading this test.
Every
> chunk appending (258, 275, 281) have matching data->RunFor(1) calls, so it
> doesn't seem clear how/when you could ever merge chunks, or why merging chunks
> would be bad for the correctness of this test.
> 
> Is this necessary? If so, it would be good to comment why - and why this test
> does not reset any values upon completion (it would seem to have global
> side-effects).

Good point. Fixed SendChunkedPost and this test to restore the original value
for merged_chunks_. Added the comment from Satish that exaplained why merging of
chunks is disabled.

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:284: data->RunFor(3);
On 2012/07/05 06:52:06, Ryan Sleevi wrote:
> As mentioned in-person, I think it would be good to also make sure that
> appending two chunks back to back, without any data->RunFor() calls in
between,
> would be good. You want to make sure that multiple calls to OnChunkAvailable
> still result in the correct behaviour within the SpdyHttpStream state machine.

Done.

https://chromiumcodereview.appspot.com/10689034/diff/1030/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:293: EXPECT_EQ(kUploadData,
std::string(buf1->data(), kUploadDataSize));
On 2012/07/05 06:52:06, Ryan Sleevi wrote:
> BUG?: It seems like these tests should be checking the result of
> ReadResponseBody() and making sure that callback was called.
> 
> This is "safe", only because |buf1| is pre-allocated to be kUploadDataSize
(line
> 291), so the copy to a string on line 293 won't read past the end.
> 
> However, since this is subtle, especially when combined with the fact that
> you're not checking that callback.callback() is called before line 295, it
makes
> me wonder whether or not the test will fail gracefully if ReadResponseBody
ever
> returned a different size.
> 
> Same for the further reads below

Added check for the callback is called.

https://chromiumcodereview.appspot.com/10689034/diff/2007/net/base/upload_dat...
File net/base/upload_data_stream.h (right):

https://chromiumcodereview.appspot.com/10689034/diff/2007/net/base/upload_dat...
net/base/upload_data_stream.h:68: // This method is provided only to be used by
unit tests.
On 2012/07/06 20:20:03, Ryan Sleevi wrote:
> nit: This method is -> These methods are
> 
> However, I'd suggest you make these private and either friend the
> forward-declared unit test fixtures directly (SpdyHttpStreamSpdy[2/3]Test and
> SpdyNetworkTransactionSpdy[2/3]test). Since individual tests derive from the
> base class, you would need to expose a protected method on the test fixtures
> that called these private methods.
> 
> Still, better than exposing these publicly, I think.

Done.

https://chromiumcodereview.appspot.com/10689034/diff/2007/net/spdy/spdy_http_...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

https://chromiumcodereview.appspot.com/10689034/diff/2007/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:287:
ASSERT_TRUE(callback.have_result());
On 2012/07/06 20:20:03, Ryan Sleevi wrote:
> I would add an
> 
> ASSERT_FALSE(callback.have_result()) on lines 280 and between 282/283
> 
> The point is that callback.have_result() should only be true once the
> UploadDataStream has indicated EOF. Until then, it should remain false -
> correct? If so, lets explicitly test that.

Done.

https://chromiumcodereview.appspot.com/10689034/diff/2007/net/spdy/spdy_http_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:296: data->RunFor(4);
On 2012/07/06 20:20:03, Ryan Sleevi wrote:
> It's not clear to me how this is better.
> 
> In particular, if for some reason all four reads are NOT consumed, this will
run
> off into infinity (or a test timeout), which is a similar-but-different BUG?
> then the one I had mentioned in the previous comment.
> 
> My thought was that (in the old patch), you'd simply add something between
lines
> 300 - 301
> int rv = http_stream->ReadResponseBody(...)
> ASSERT_EQ(kUploadDataSize, rv);
> EXPECT_EQ(...)
> 
> The fact is, ReadResponseBody takes a net::CompletionCallback, therefore it's
> valid for it to return ERR_IO_PENDING. This test assumes that the only way
> ERR_IO_PENDING can be returned is if the network IO is slow - but I don't
think
> that's at all guaranteed by the interface.
> 
> Because this test is not setup to handle ERR_IO_PENDING, you should explicitly
> ASSERT you only ever get your expected result.
> 
> I also think your RunFor(1) + those ASSERTs are safer than RunFor(4), since
the
> RunFor(4) assumes (again, not guaranteed by the interface) that the SpdyStream
> will consume all data available to it from the network. I don't know if that's
> guaranteed, since there is a degree of internal buffering that happens.

Done.

[Ryan Sleevi, 2012-07-06 23:15:49]

http://codereview.chromium.org/10689034/diff/16020/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/10689034/diff/16020/net/base/upload_data_strea...
net/base/upload_data_stream.h:82:
FRIEND_TEST_ALL_PREFIXES(SpdyNetworkTransactionSpdy3Test, ChunkedPost);
Sorry, what I was suggesting was an either/or.

either:
Your base test (SpdyHttpStreamSpdy[2/3]Test, etc) could have a protected member
(SetMergeUploadChunks?) that calls UploadDataStream::set_merge_chunks. You need
only friend the base test fixtures (meaning you only need lines 73-76)

or:
You explicitly FRIEND_TEST_ALL_PREFIXES the tests that directly call
UploadDataStream::set_merge_chunks (meaning you only need lines 77-82)

You don't need both here.

If you do keep FRIEND_TEST_ALL_PREFIXES, then you also need to include
base/gtest_prod_util.h here (line 8), and you don't need the forward declares
(line 17-20/73-76)

If you keep the "friend class" (73-76) and drop the FRIEND_TEST, then you don't
need the declarations on line 17-20. A "friend class" statement acts as a
forward declaration of a type-id, within the same naming scope, if that type-id
is not yet in scope, and will act is you had done what you're explicitly doing
on 17-20.

[ramant (doing other things), 2012-07-07 04:47:05]

Hi Ryan Sleevi,
  Thanks much for your comments. Made the changes to upload_data_stream.h per
your suggestion.

-raman

http://codereview.chromium.org/10689034/diff/16020/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/10689034/diff/16020/net/base/upload_data_strea...
net/base/upload_data_stream.h:82:
FRIEND_TEST_ALL_PREFIXES(SpdyNetworkTransactionSpdy3Test, ChunkedPost);
On 2012/07/06 23:15:49, Ryan Sleevi wrote:
> Sorry, what I was suggesting was an either/or.
> 
> either:
> Your base test (SpdyHttpStreamSpdy[2/3]Test, etc) could have a protected
member
> (SetMergeUploadChunks?) that calls UploadDataStream::set_merge_chunks. You
need
> only friend the base test fixtures (meaning you only need lines 73-76)
> 
> or:
> You explicitly FRIEND_TEST_ALL_PREFIXES the tests that directly call
> UploadDataStream::set_merge_chunks (meaning you only need lines 77-82)
> 
> You don't need both here.
> 
> If you do keep FRIEND_TEST_ALL_PREFIXES, then you also need to include
> base/gtest_prod_util.h here (line 8), and you don't need the forward declares
> (line 17-20/73-76)
> 
> If you keep the "friend class" (73-76) and drop the FRIEND_TEST, then you
don't
> need the declarations on line 17-20. A "friend class" statement acts as a
> forward declaration of a type-id, within the same naming scope, if that
type-id
> is not yet in scope, and will act is you had done what you're explicitly doing
> on 17-20.

Done. Thanks much for your detailed comments.

[Ryan Hamilton, 2012-07-09 17:52:02]

So as I understand the history of this CL, it includes fixes for two known bugs:

1) Window updates are ignored under some circumstances (the bug in the report)
2) The goofy state machinery which confused a new chunk being available with a
write completing.

Can you confirm that your new tests explicitly cover these cases AND fail with
the old code?

http://codereview.chromium.org/10689034/diff/10027/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/10689034/diff/10027/net/base/upload_data_strea...
net/base/upload_data_stream.h:75: static bool merge_chunks() { return
merge_chunks_; }
Instead of making exposing this method, and having each of the four tests call
it, save away the state, and then call set_merge_chunks() with this saved value,
you might consider adding a method like ResetMergeChunks (or some such) which
resets this value to the default.  This would be similar to what we do with
SpdySession.

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:103: // SpdyStream::WriteStreamData. In the case of
chunked uploads, if data is not
nit:  can you change "SpdyStream::WriteStreamData" to WriteStreamData on the
underlying SpdyStream?

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:104: // available, returns ERR_IO_PENDING, and the
write will be completed at a
Can you add a comment that explains the return value in the case that the upload
is not chunked?

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream_...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:128: // TODO(satish): Remove this
once we have a better way to unit test POST
Why is this TODO owned by satish?

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_spdy2_unittest.cc:1610: // TODO(satish):
Remove this once we have a better way to unit test POST
satish?

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:1617: // TODO(satish):
Remove this once we have a better way to unit test POST
satish?

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

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:169: if (io_state_ != STATE_SEND_BODY_COMPLETE)
I don't like this.  While it is OK (mebe) to receive a WINDOW_UPDATE frame on a
closed SpdyStream (and hence it is safe to silently ignore it) it is NOT OK to
receive a WINDOW_UPDATE frame before we have written the request headers.  In
this case, we need to either process the update or error out, not silently
ignore it.

[ramant (doing other things), 2012-07-11 01:36:18]

Hi Ryan Hamilton,
  Wrote a test to test that the failure condition of old code when we receive a
WINDOW_UPDATE during chunk uploading (thanks for your suggestion to where to add
the reading of WINDOW_UPDATE frame).

  Also verified that the old code works if were to change SpdyStream's
IncreaseSendWindowSize to the following condition.
  if (io_state_ != STATE_SEND_BODY_COMPLETE && io_state_ != STATE_SEND_BODY)

thanks,
raman

http://codereview.chromium.org/10689034/diff/10027/net/base/upload_data_stream.h
File net/base/upload_data_stream.h (right):

http://codereview.chromium.org/10689034/diff/10027/net/base/upload_data_strea...
net/base/upload_data_stream.h:75: static bool merge_chunks() { return
merge_chunks_; }
On 2012/07/09 17:52:02, Ryan Hamilton wrote:
> Instead of making exposing this method, and having each of the four tests call
> it, save away the state, and then call set_merge_chunks() with this saved
value,
> you might consider adding a method like ResetMergeChunks (or some such) which
> resets this value to the default.  This would be similar to what we do with
> SpdySession.

Done.

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:103: // SpdyStream::WriteStreamData. In the case of
chunked uploads, if data is not
On 2012/07/09 17:52:02, Ryan Hamilton wrote:
> nit:  can you change "SpdyStream::WriteStreamData" to WriteStreamData on the
> underlying SpdyStream?

Done.

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:104: // available, returns ERR_IO_PENDING, and the
write will be completed at a
On 2012/07/09 17:52:02, Ryan Hamilton wrote:
> Can you add a comment that explains the return value in the case that the
upload
> is not chunked?

Done.

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream_...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:128: // TODO(satish): Remove this
once we have a better way to unit test POST
On 2012/07/09 17:52:02, Ryan Hamilton wrote:
> Why is this TODO owned by satish?

Done.

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_spdy2_unittest.cc:1610: // TODO(satish):
Remove this once we have a better way to unit test POST
On 2012/07/09 17:52:02, Ryan Hamilton wrote:
> satish?

Done.

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:1617: // TODO(satish):
Remove this once we have a better way to unit test POST
On 2012/07/09 17:52:02, Ryan Hamilton wrote:
> satish?

Done.

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

http://codereview.chromium.org/10689034/diff/10027/net/spdy/spdy_stream.cc#ne...
net/spdy/spdy_stream.cc:169: if (io_state_ != STATE_SEND_BODY_COMPLETE)
On 2012/07/09 17:52:02, Ryan Hamilton wrote:
> I don't like this.  While it is OK (mebe) to receive a WINDOW_UPDATE frame on
a
> closed SpdyStream (and hence it is safe to silently ignore it) it is NOT OK to
> receive a WINDOW_UPDATE frame before we have written the request headers.  In
> this case, we need to either process the update or error out, not silently
> ignore it.

Done.

[Ryan Hamilton, 2012-07-11 17:11:17]

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:127: // requests with chunked
uploads.
Please remove this comment.  (If you really feel that you need *some* comment,
then add a comment that explains the purpose of this code.  But personally, I
feel no need to say that we need a "Better" way to do these tests)  Please do
this in all unitest.cc files.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:188: TEST_F(SpdyHttpStreamSpdy2Test,
DelayedSendChunkedPost) {
Please add a comment before this tests that explains what we're testing here. 
Is this test attempting to catch the bug that sleevi pointed out and you fixed
with the OnChunkAvailable calling OnWriteComplete?  If so, the comment should
probably talk about a Chunk being made available while a write is outstanding.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
File net/spdy/spdy_http_stream_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:345: TEST_F(SpdyHttpStreamSpdy3Test,
DelayedSendChunkedPostWithWindowUpdate) {
Please add a comment here that explains we're testing that receipt of a
WINDOW_UPDATE frame while we're waiting for a chunk to be made available is
handled correctly.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:442:
ASSERT_TRUE(callback.have_result());
I don't understand why all of these checkes for callback.have_results() are for.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2072:
CreateMockRead(*reply),
Why is this change required?  Since the test used to pass, I suspect this is
indicative of a bug.

[Ryan Sleevi, 2012-07-11 17:43:25]

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:286:
ASSERT_TRUE(callback.have_result());
drop this

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:290:
ASSERT_TRUE(callback.have_result());
drop this

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:294:
ASSERT_TRUE(callback.have_result());
drop this

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:299: EXPECT_GT(rv, 0);
move 298+299 -> 283

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:300: EXPECT_EQ(OK,
http_stream->ReadResponseHeaders(callback.callback()));
ASSERT_EQ

[ramant (doing other things), 2012-07-11 19:40:43]

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:127: // requests with chunked
uploads.
On 2012/07/11 17:11:19, Ryan Hamilton wrote:
> Please remove this comment.  (If you really feel that you need *some* comment,
> then add a comment that explains the purpose of this code.  But personally, I
> feel no need to say that we need a "Better" way to do these tests)  Please do
> this in all unitest.cc files.

Done.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:188: TEST_F(SpdyHttpStreamSpdy2Test,
DelayedSendChunkedPost) {
On 2012/07/11 17:11:19, Ryan Hamilton wrote:
> Please add a comment before this tests that explains what we're testing here. 
> Is this test attempting to catch the bug that sleevi pointed out and you fixed
> with the OnChunkAvailable calling OnWriteComplete?  If so, the comment should
> probably talk about a Chunk being made available while a write is outstanding.

Done.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:286:
ASSERT_TRUE(callback.have_result());
On 2012/07/11 17:43:25, Ryan Sleevi wrote:
> drop this

Done.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:290:
ASSERT_TRUE(callback.have_result());
On 2012/07/11 17:43:25, Ryan Sleevi wrote:
> drop this

Done.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:294:
ASSERT_TRUE(callback.have_result());
On 2012/07/11 17:43:25, Ryan Sleevi wrote:
> drop this

Done.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:299: EXPECT_GT(rv, 0);
On 2012/07/11 17:43:25, Ryan Sleevi wrote:
> move 298+299 -> 283

Done.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy2_unittest.cc:300: EXPECT_EQ(OK,
http_stream->ReadResponseHeaders(callback.callback()));
On 2012/07/11 17:43:25, Ryan Sleevi wrote:
> ASSERT_EQ

Done.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
File net/spdy/spdy_http_stream_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:345: TEST_F(SpdyHttpStreamSpdy3Test,
DelayedSendChunkedPostWithWindowUpdate) {
On 2012/07/11 17:11:19, Ryan Hamilton wrote:
> Please add a comment here that explains we're testing that receipt of a
> WINDOW_UPDATE frame while we're waiting for a chunk to be made available is
> handled correctly.

Done.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_http_stream_...
net/spdy/spdy_http_stream_spdy3_unittest.cc:442:
ASSERT_TRUE(callback.have_result());
On 2012/07/11 17:11:19, Ryan Hamilton wrote:
> I don't understand why all of these checkes for callback.have_results() are
for.
>  

Done.

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_network_tran...
File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/16054/net/spdy/spdy_network_tran...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2072:
CreateMockRead(*reply),
On 2012/07/11 17:11:19, Ryan Hamilton wrote:
> Why is this change required?  Since the test used to pass, I suspect this is
> indicative of a bug.

As we have discussed, re-implemented this test using DeterministicSocketData.

[Ryan Sleevi, 2012-07-11 19:54:25]

http://codereview.chromium.org/10689034/diff/4044/net/spdy/spdy_network_trans...
File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/4044/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2090:
EXPECT_EQ(ERR_IO_PENDING, rv);
nit: ASSERT_EQ

if !pending, then data->RunFor() may hang.

http://codereview.chromium.org/10689034/diff/4044/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2092: data->RunFor(6);
Could you add a comment why 6, since you only have 5 total mock read/writes?

http://codereview.chromium.org/10689034/diff/4044/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2093: rv =
callback.WaitForResult();
nit: ASSERT_TRUE(callback.have_result()) between 2092 and 2093

[ramant (doing other things), 2012-07-11 20:52:20]

http://codereview.chromium.org/10689034/diff/4044/net/spdy/spdy_network_trans...
File net/spdy/spdy_network_transaction_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/4044/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2090:
EXPECT_EQ(ERR_IO_PENDING, rv);
On 2012/07/11 19:54:25, Ryan Sleevi wrote:
> nit: ASSERT_EQ
> 
> if !pending, then data->RunFor() may hang.

Done.

http://codereview.chromium.org/10689034/diff/4044/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2092: data->RunFor(6);
On 2012/07/11 19:54:25, Ryan Sleevi wrote:
> Could you add a comment why 6, since you only have 5 total mock read/writes?
 
Done.

http://codereview.chromium.org/10689034/diff/4044/net/spdy/spdy_network_trans...
net/spdy/spdy_network_transaction_spdy3_unittest.cc:2093: rv =
callback.WaitForResult();
On 2012/07/11 19:54:25, Ryan Sleevi wrote:
> nit: ASSERT_TRUE(callback.have_result()) between 2092 and 2093

Done.

[Ryan Hamilton, 2012-07-11 21:08:21]

Excellent progress!  Only nits ...

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:104: // (which always returns ERR_IO_PENDING). In
the case of chunked uploads, if
I'm not sure that you need any of this comment after "underlying SpdyStream",
(with the possible exception of "This method always returns ERR_IO_PENDING"
though that is only true as long as the implementation of
SpdyStream::WriteStreamData does not change.)

sleevi, what do you think?

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:188: // asynchronously, over a
socket where writes may also complete asynchronously.
nit:  ... does not get confused when a chunk becomes available while a write is
pending.

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:284: // Now append the second chunk.
This will enqueue another write.
nit: // Now append the final two chunks which will enqueue two more writes.
(And then remove the comment from the next line)

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:303: ASSERT_EQ(kUploadDataSize, rv);
nit:  I would rewrite this as:
ASSERT_EQ(kUploadDataSize,
         http_stream->ReadResponseBody(buf1,
                                          kUploadDataSize,
                                          callback.callback()));

(and on subsequent lines)  but this is your call.

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:197: TEST_F(SpdyHttpStreamSpdy3Test,
DelayedSendChunkedPost) {
same comments as in _spdy2

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:453: // Now append the second chunk.
This will enqueue another write.
nit: it looks like we don't need the 2nd or third chunks for this test.  do you
agree?  if so, can you nuke them?

[Ryan Sleevi, 2012-07-11 21:13:46]

Changes LGTM, and the change you proposed SGTM.

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:104: // (which always returns ERR_IO_PENDING). In
the case of chunked uploads, if
On 2012/07/11 21:08:21, Ryan Hamilton wrote:
> I'm not sure that you need any of this comment after "underlying SpdyStream",
> (with the possible exception of "This method always returns ERR_IO_PENDING"
> though that is only true as long as the implementation of
> SpdyStream::WriteStreamData does not change.)
> 
> sleevi, what do you think?

Sounds good - I agree that we don't want to document SpdyStream's behaviour in
this header "(which always returns)", nor do I think we should say "This method
always returns"

Just documenting how this method behaves, and everything else is unspecified/as
implemented by the underlying SpdyStream.

[ramant (doing other things), 2012-07-11 22:02:21]

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream.h...
net/spdy/spdy_http_stream.h:104: // (which always returns ERR_IO_PENDING). In
the case of chunked uploads, if
On 2012/07/11 21:08:21, Ryan Hamilton wrote:
> I'm not sure that you need any of this comment after "underlying SpdyStream",
> (with the possible exception of "This method always returns ERR_IO_PENDING"
> though that is only true as long as the implementation of
> SpdyStream::WriteStreamData does not change.)
> 
> sleevi, what do you think?

Done.

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy2_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:188: // asynchronously, over a
socket where writes may also complete asynchronously.
On 2012/07/11 21:08:21, Ryan Hamilton wrote:
> nit:  ... does not get confused when a chunk becomes available while a write
is
> pending.

Done.

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:284: // Now append the second chunk.
This will enqueue another write.
On 2012/07/11 21:08:21, Ryan Hamilton wrote:
> nit: // Now append the final two chunks which will enqueue two more writes.
> (And then remove the comment from the next line)

Done.

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy2_unittest.cc:303: ASSERT_EQ(kUploadDataSize, rv);
On 2012/07/11 21:08:21, Ryan Hamilton wrote:
> nit:  I would rewrite this as:
> ASSERT_EQ(kUploadDataSize,
>          http_stream->ReadResponseBody(buf1,
>                                           kUploadDataSize,
>                                           callback.callback()));
> 
> (and on subsequent lines)  but this is your call.

Done.

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:197: TEST_F(SpdyHttpStreamSpdy3Test,
DelayedSendChunkedPost) {
On 2012/07/11 21:08:21, Ryan Hamilton wrote:
> same comments as in _spdy2

Done.

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:453: // Now append the second chunk.
This will enqueue another write.
On 2012/07/11 21:08:21, Ryan Hamilton wrote:
> nit: it looks like we don't need the 2nd or third chunks for this test.  do
you
> agree?  if so, can you nuke them?

Done.

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:453: // Now append the second chunk.
This will enqueue another write.
On 2012/07/11 21:08:21, Ryan Hamilton wrote:
> nit: it looks like we don't need the 2nd or third chunks for this test.  do
you
> agree?  if so, can you nuke them?

Deleted the third chunk. We need the 2nd chunk to duplicate the bug.

[Ryan Hamilton, 2012-07-11 22:51:14]

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
File net/spdy/spdy_http_stream_spdy3_unittest.cc (right):

http://codereview.chromium.org/10689034/diff/8065/net/spdy/spdy_http_stream_s...
net/spdy/spdy_http_stream_spdy3_unittest.cc:453: // Now append the second chunk.
This will enqueue another write.
On 2012/07/11 22:02:21, ramant wrote:
> On 2012/07/11 21:08:21, Ryan Hamilton wrote:
> > nit: it looks like we don't need the 2nd or third chunks for this test.  do
> you
> > agree?  if so, can you nuke them?
> 
> Deleted the third chunk. We need the 2nd chunk to duplicate the bug.

Why is the 2nd chunk required?  We don't AppendChunk() it until *After* we
verify that we handled the window update.

http://codereview.chromium.org/10689034/diff/13032/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/13032/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:104: // This method always returns ERR_IO_PENDING.
nit: please remove this final sentence

[ramant (doing other things), 2012-07-11 23:00:21]

http://codereview.chromium.org/10689034/diff/13032/net/spdy/spdy_http_stream.h
File net/spdy/spdy_http_stream.h (right):

http://codereview.chromium.org/10689034/diff/13032/net/spdy/spdy_http_stream....
net/spdy/spdy_http_stream.h:104: // This method always returns ERR_IO_PENDING.
On 2012/07/11 22:51:15, Ryan Hamilton wrote:
> nit: please remove this final sentence 

Done.

[Ryan Hamilton, 2012-07-11 23:05:15]

LGTM - Looks like the 29th time was the charm :>

[commit-bot: I haz the power, 2012-07-11 23:20:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rtenneti@chromium.org/10689034/13035

[commit-bot: I haz the power, 2012-07-12 01:00:45]

Change committed as 146255