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

net/spdy/spdy_http_stream_spdy2_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/spdy/spdy_http_stream.h"
 
 #include "crypto/ec_private_key.h"
 #include "crypto/ec_signature_creator.h"
 #include "crypto/signature_creator.h"
 #include "net/base/asn1_util.h"
@@ skipping 106 matching lines @@
   EXPECT_TRUE(data()->at_write_eof());
 }
 
 TEST_F(SpdyHttpStreamSpdy2Test, SendChunkedPost) {
   UploadDataStream::set_merge_chunks(false);
 
   scoped_ptr<SpdyFrame> req(ConstructChunkedSpdyPost(NULL, 0));
   scoped_ptr<SpdyFrame> chunk1(ConstructSpdyBodyFrame(1, false));
   scoped_ptr<SpdyFrame> chunk2(ConstructSpdyBodyFrame(1, true));
   MockWrite writes[] = {
-    CreateMockWrite(*req.get(), 1),
-    CreateMockWrite(*chunk1, 2),  // POST upload frames
-    CreateMockWrite(*chunk2, 3),
+    CreateMockWrite(*req.get(), 0),
+    CreateMockWrite(*chunk1, 1),  // POST upload frames
+    CreateMockWrite(*chunk2, 2),
   };
   scoped_ptr<SpdyFrame> resp(ConstructSpdyPostSynReply(NULL, 0));
   MockRead reads[] = {
-    CreateMockRead(*resp, 4),
-    CreateMockRead(*chunk1, 5),
+    CreateMockRead(*resp, 3),
+    CreateMockRead(*chunk1, 4),
     CreateMockRead(*chunk2, 5),
     MockRead(SYNCHRONOUS, 0, 6)  // EOF
   };
 
   HostPortPair host_port_pair("www.google.com", 80);
   HostPortProxyPair pair(host_port_pair, ProxyServer::Direct());
   EXPECT_EQ(OK, InitSession(reads, arraysize(reads), writes, arraysize(writes),
                             host_port_pair));
 
   HttpRequestInfo request;
@@ skipping 26 matching lines @@
   data()->CompleteRead();
   MessageLoop::current()->RunAllPending();
 
   // Because we abandoned the stream, we don't expect to find a session in the
   // pool anymore.
   EXPECT_FALSE(http_session_->spdy_session_pool()->HasSession(pair));
   EXPECT_TRUE(data()->at_read_eof());
   EXPECT_TRUE(data()->at_write_eof());
 }
 
+TEST_F(SpdyHttpStreamSpdy2Test, DelayedSendChunkedPost) {
+  UploadDataStream::set_merge_chunks(false);

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

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).

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

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.

+
+  const char kUploadData1[] = "12345678";
+  const int kUploadData1Size = arraysize(kUploadData1)-1;
+  scoped_ptr<SpdyFrame> req(ConstructChunkedSpdyPost(NULL, 0));
+  scoped_ptr<SpdyFrame> chunk1(ConstructSpdyBodyFrame(1, false));
+  scoped_ptr<SpdyFrame> chunk2(
+      ConstructSpdyBodyFrame(1, kUploadData1, kUploadData1Size, false));
+  scoped_ptr<SpdyFrame> chunk3(ConstructSpdyBodyFrame(1, true));
+  MockWrite writes[] = {
+    CreateMockWrite(*req.get(), 0),
+    CreateMockWrite(*chunk1, 1),  // POST upload frames
+    CreateMockWrite(*chunk2, 2),
+    CreateMockWrite(*chunk3, 3),
+  };
+  scoped_ptr<SpdyFrame> resp(ConstructSpdyPostSynReply(NULL, 0));
+  MockRead reads[] = {
+    CreateMockRead(*resp, 4),
+    CreateMockRead(*chunk1, 5),
+    CreateMockRead(*chunk2, 6),
+    CreateMockRead(*chunk3, 7),
+    MockRead(ASYNC, 0, 8)  // EOF
+  };
+
+  HostPortPair host_port_pair("www.google.com", 80);
+  HostPortProxyPair pair(host_port_pair, ProxyServer::Direct());
+
+  scoped_refptr<DeterministicSocketData> data(
+      new DeterministicSocketData(reads, arraysize(reads),
+                                  writes, arraysize(writes)));
+
+  DeterministicMockClientSocketFactory* socket_factory =
+      session_deps_.deterministic_socket_factory.get();
+  socket_factory->AddSocketDataProvider(data.get());
+
+  http_session_ = SpdySessionDependencies::SpdyCreateSessionDeterministic(
+      &session_deps_);
+  session_ = http_session_->spdy_session_pool()->Get(pair, BoundNetLog());
+  transport_params_ = new TransportSocketParams(host_port_pair,
+                                                MEDIUM, false, false,
+                                                OnHostResolutionCallback());
+
+  TestCompletionCallback callback;
+  scoped_ptr<ClientSocketHandle> connection(new ClientSocketHandle);
+
+  EXPECT_EQ(ERR_IO_PENDING,
+            connection->Init(host_port_pair.ToString(),
+                             transport_params_,
+                             MEDIUM,
+                             callback.callback(),
+                             http_session_->GetTransportSocketPool(
+                                 HttpNetworkSession::NORMAL_SOCKET_POOL),
+                             BoundNetLog()));
+
+  callback.WaitForResult();
+  EXPECT_EQ(OK,
+            session_->InitializeWithSocket(connection.release(), false, OK));
+
+  HttpRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://www.google.com/");
+  request.upload_data = new UploadData();
+  request.upload_data->set_is_chunked(true);
+
+  BoundNetLog net_log;
+  scoped_ptr<SpdyHttpStream> http_stream(
+      new SpdyHttpStream(session_.get(), true));
+  ASSERT_EQ(OK,
+            http_stream->InitializeStream(&request,
+                                          net_log,
+                                          CompletionCallback()));
+
+  scoped_ptr<UploadDataStream> upload_stream(
+      new UploadDataStream(request.upload_data));
+  ASSERT_EQ(OK, upload_stream->Init());
+
+  request.upload_data->AppendChunk(kUploadData, kUploadDataSize, false);
+
+  HttpRequestHeaders headers;
+  HttpResponseInfo response;
+  // This will attempt to Write() the initial request and headers, which will
+  // complete asynchronously.
+  EXPECT_EQ(ERR_IO_PENDING,
+            http_stream->SendRequest(headers,
+                                     upload_stream.Pass(),
+                                     &response,
+                                     callback.callback()));
+  EXPECT_TRUE(http_session_->spdy_session_pool()->HasSession(pair));
+
+  // Complete the initial request write and the first chunk.
+  data->RunFor(1);
+
+  // Now append the second chunk. This will enqueue another write.
+  request.upload_data->AppendChunk(kUploadData1, kUploadData1Size, false);
+
+  // Complete the second chunk.
+  data->RunFor(1);
+
+  // Now append final chunk. This will enqueue another write.
+  request.upload_data->AppendChunk(kUploadData, kUploadDataSize, true);
+
+  // Finalize writing the last chunk, which will enqueue the trailer.
+  data->RunFor(3);

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

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.

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

Done.

+  int rv = callback.WaitForResult();
+  EXPECT_GT(rv, 0);
+  EXPECT_EQ(OK, http_stream->ReadResponseHeaders(callback.callback()));
+
+  // Finish reading |chunk1|, |chunk2| and |chunk3|.
+  data->RunFor(1);
+  scoped_refptr<IOBuffer> buf1(new IOBuffer(kUploadDataSize));
+  http_stream->ReadResponseBody(buf1, kUploadDataSize, callback.callback());
+  EXPECT_EQ(kUploadData, std::string(buf1->data(), kUploadDataSize));

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

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

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

Added check for the callback is called.

+
+  data->RunFor(1);

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

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]

+  scoped_refptr<IOBuffer> buf2(new IOBuffer(kUploadData1Size));
+  http_stream->ReadResponseBody(buf2, kUploadData1Size, callback.callback());
+  EXPECT_EQ(kUploadData1, std::string(buf2->data(), kUploadData1Size));
+
+  data->RunFor(1);
+  scoped_refptr<IOBuffer> buf3(new IOBuffer(kUploadDataSize));
+  http_stream->ReadResponseBody(buf3, kUploadDataSize, callback.callback());
+  EXPECT_EQ(kUploadData, std::string(buf3->data(), kUploadDataSize));
+
+  // Finish reading the |EOF|.
+  data->RunFor(1);
+  ASSERT_TRUE(response.headers.get());
+  ASSERT_EQ(200, response.headers->response_code());
+  EXPECT_TRUE(data->at_read_eof());
+  EXPECT_TRUE(data->at_write_eof());
+}
+
 // Test case for bug: http://code.google.com/p/chromium/issues/detail?id=50058
 TEST_F(SpdyHttpStreamSpdy2Test, SpdyURLTest) {
   const char * const full_url = "http://www.google.com/foo?query=what#anchor";
   const char * const base_url = "http://www.google.com/foo?query=what";
   scoped_ptr<SpdyFrame> req(ConstructSpdyGet(base_url, false, 1, LOWEST));
   MockWrite writes[] = {
     CreateMockWrite(*req.get(), 1),
   };
   scoped_ptr<SpdyFrame> resp(ConstructSpdyGetSynReply(NULL, 0, 1));
   MockRead reads[] = {
@@ skipping 40 matching lines @@
   // pool anymore.
   EXPECT_FALSE(http_session_->spdy_session_pool()->HasSession(pair));
   EXPECT_TRUE(data()->at_read_eof());
   EXPECT_TRUE(data()->at_write_eof());
 }
 
 // TODO(willchan): Write a longer test for SpdyStream that exercises all
 // methods.
 
 }  // namespace net