Introduce error handling in HttpStreamParser on UploadDataStream::Read() failure.

net/http/http_stream_parser_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/http/http_stream_parser.h"
 
 #include <stdint.h>
 
 #include <algorithm>
 #include <memory>
@@ skipping 28 matching lines @@
 
 namespace net {
 
 namespace {
 
 const size_t kOutputSize = 1024;  // Just large enough for this test.
 // The number of bytes that can fit in a buffer of kOutputSize.
 const size_t kMaxPayloadSize =
     kOutputSize - HttpStreamParser::kChunkHeaderFooterSize;
 
+const bool kAsynchronously = true;

[mmenke, 2016/05/24 17:13:47]

I'd suggest using an enum, and making the enum a public member of
ElementsUploadDataStreamError.

Could even make it an enum class, which is more defensive - protects against
accidentally using it where you want a real bool.  The downside of enum classes
is they're wordy, but think the benefits outweigh the cost, in this case.  So
could do something like:

enum class FailureMode {
  SYNC,
  ASYNC
};

And then, outside of ElementsUploadDataStreamError, it would be referenced as,
for example, ElementsUploadDataStreamError::FailureMode::SYNC.  (You may know
all this, just mentioning it because this C++0x11 stuff is fairly new to me, at
least)

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+
 // Helper method to create a connected ClientSocketHandle using |data|.
 // Modifies |data|.
 std::unique_ptr<ClientSocketHandle> CreateConnectedSocketHandle(
     SequencedSocketData* data) {
   data->set_connect_data(MockConnect(SYNCHRONOUS, OK));
 
   std::unique_ptr<MockTCPClientSocket> socket(
       new MockTCPClientSocket(net::AddressList(), nullptr, data));
 
   TestCompletionCallback callback;
   EXPECT_EQ(OK, socket->Connect(callback.callback()));
 
   std::unique_ptr<ClientSocketHandle> socket_handle(new ClientSocketHandle);
   socket_handle->SetSocket(std::move(socket));
   return socket_handle;
 }
 
+class ElementsUploadDataStreamError : public ElementsUploadDataStream {

[mmenke, 2016/05/24 17:13:47]

Also, this is an UploadDataStream with a read error, rather than the error
itself.  Think a name like "UploadDataStreamWithReadError" or
"ReadErrorUploadDataStream" would be clearer.

[mmenke, 2016/05/24 17:13:47]

This should inherit from UploadDataStream, instead of ElementsUploadDataStream.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+ public:
+  ElementsUploadDataStreamError(
+      std::vector<std::unique_ptr<UploadElementReader>> element_readers,
+      int64_t identifier,
+      bool async = false)

[mmenke, 2016/05/24 17:13:48]

There's a preference not to use default values, in net, to make code clearer.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+      : ElementsUploadDataStream(std::move(element_readers), identifier) {
+    async_ = async;
+  }
+
+  int ReadInternal(IOBuffer* buf, int buf_len) override {
+    if (async_)
+      return ERR_IO_PENDING;

[mmenke, 2016/05/24 17:13:47]

For the async case, suggest instead:

base::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
    base::Bind(&ElementsUploadDataStreamError::CompleteRead,
               weak_factory_.GetWeakPtr()));
return ERR_IO_PENDING;

That will make it automatically run CompleteRead asynchronously, and
CompleteRead can then be private.  To see how to set up weak_factory_, can just
do a search for "weak_factory_" in the Chrome repo.  It basically makes is to if
the object is deleted before the bound method runs, the callback will be
cancelled, instead of causing a crash.  Doesn't really matter for the tests
you're adding, since they don't delete the object without waiting for the
callback, but think it's best to be safe.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+    return ERR_FILE_READ_FAIL;

[mmenke, 2016/05/24 17:13:48]

This can be any error, per my suggestion not to add this error code.  Could just
use ERR_FAILED, for instance, which is used in a lot of obscure cases, not worth
their own error code.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+  }
+
+  void CompleteRead(int result) {

[mmenke, 2016/05/24 17:13:47]

Don't think this should take an argument, since ReadInternal returns a fixed
error code.  Best to be consistent.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+    ElementsUploadDataStream::OnReadCompleted(result);
+  }
+
+ private:
+  bool IsInMemory() const override { return false; }

[mmenke, 2016/05/24 17:13:47]

nit:  This should be up with the other override method.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+  bool async_;

[mmenke, 2016/05/24 17:13:47]

nit:

DISALLOW_COPY_AND_ASSIGN(ElementsUploadDataStream);

[mmenke, 2016/05/24 17:13:47]

nit: const

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+};
+
+TEST(HttpStreamParser, DataReadErrorSynchronous) {
+  MockWrite writes[] = {
+      MockWrite(SYNCHRONOUS, 0, "POST / HTTP/1.1\r\n"),
+      MockWrite(SYNCHRONOUS, 1, "Content-Length: 12\r\n\r\n"),
+  };
+
+  SequencedSocketData data(nullptr, 0, writes, arraysize(writes));
+  std::unique_ptr<ClientSocketHandle> socket_handle =
+      CreateConnectedSocketHandle(&data);
+
+  std::vector<std::unique_ptr<UploadElementReader>> element_readers;
+
+  // "hello world!" shouldn't be read nor sent.
+  // ElementsUploadDataStreamError::ReadInternal() emulates either pending IO
+  // or ERR_FILE_READ_FAIL
+  element_readers.push_back(
+      base::WrapUnique(new UploadBytesElementReader("hello world!", 12)));
+  ElementsUploadDataStreamError upload_data_stream(std::move(element_readers),
+                                                   0);
+  ASSERT_EQ(OK, upload_data_stream.Init(TestCompletionCallback().callback()));
+
+  HttpRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://localhost");
+  request.upload_data_stream = &upload_data_stream;
+
+  scoped_refptr<GrowableIOBuffer> read_buffer(new GrowableIOBuffer);
+  HttpStreamParser parser(socket_handle.get(), &request, read_buffer.get(),
+                          BoundNetLog());
+
+  HttpRequestHeaders headers;
+  headers.SetHeader("Content-Length", "12");
+
+  HttpResponseInfo response;
+  TestCompletionCallback callback;
+  EXPECT_EQ(ERR_FILE_READ_FAIL,
+            parser.SendRequest("POST / HTTP/1.1\r\n", headers, &response,
+                               callback.callback()));

[mmenke, 2016/05/24 17:13:47]

This works, and is correct, but I suggest instead:

int result = parser.SendRequest(...);
EXPECT_EQ(ERR_..., callback.GetResult(result));

This works if parser returns a result asynchronously for any reason, and can use
it for the next test as well, if you follow my suggestions above.

[maksims (do not use this acc), 2016/05/25 07:13:38]

Done.

+
+  EXPECT_EQ(CountWriteBytes(writes, arraysize(writes)), parser.sent_bytes());
+}
+
+TEST(HttpStreamParser, DataReadErrorAsynchronous) {
+  MockWrite writes[] = {
+      MockWrite(ASYNC, 0, "POST / HTTP/1.1\r\n"),
+      MockWrite(ASYNC, 1, "Content-Length: 12\r\n\r\n"),
+  };
+
+  SequencedSocketData data(nullptr, 0, writes, arraysize(writes));
+  std::unique_ptr<ClientSocketHandle> socket_handle =
+      CreateConnectedSocketHandle(&data);
+
+  std::vector<std::unique_ptr<UploadElementReader>> element_readers;
+
+  // "hello world!" shouldn't be read nor sent.
+  // ElementsUploadDataStreamError::ReadInternal() emulates either pending IO
+  // or ERR_FILE_READ_FAIL
+  element_readers.push_back(
+      base::WrapUnique(new UploadBytesElementReader("hello world!", 12)));
+  ElementsUploadDataStreamError upload_data_stream(std::move(element_readers),
+                                                   0, kAsynchronously);
+  ASSERT_EQ(OK, upload_data_stream.Init(TestCompletionCallback().callback()));
+
+  HttpRequestInfo request;
+  request.method = "POST";
+  request.url = GURL("http://localhost");
+  request.upload_data_stream = &upload_data_stream;
+
+  scoped_refptr<GrowableIOBuffer> read_buffer(new GrowableIOBuffer);
+  HttpStreamParser parser(socket_handle.get(), &request, read_buffer.get(),
+                          BoundNetLog());
+
+  HttpRequestHeaders headers;
+  headers.SetHeader("Content-Length", "12");
+
+  DLOG(ERROR) << "TUT";

[mmenke, 2016/05/24 17:13:48]

Remove debugging code.

+  HttpResponseInfo response;
+  TestCompletionCallback callback;
+  EXPECT_EQ(ERR_IO_PENDING, parser.SendRequest("POST / HTTP/1.1\r\n", headers,
+                                               &response, callback.callback()));
+
+  base::RunLoop().RunUntilIdle();
+  upload_data_stream.CompleteRead(ERR_FILE_READ_FAIL);
+  EXPECT_EQ(ERR_FILE_READ_FAIL, callback.WaitForResult());
+  EXPECT_EQ(CountWriteBytes(writes, arraysize(writes)), parser.sent_bytes());
+}
+
 // The empty payload is how the last chunk is encoded.
 TEST(HttpStreamParser, EncodeChunk_EmptyPayload) {
   char output[kOutputSize];
 
   const base::StringPiece kPayload = "";
   const base::StringPiece kExpected = "0\r\n\r\n";
   const int num_bytes_written =
       HttpStreamParser::EncodeChunk(kPayload, output, sizeof(output));
   ASSERT_EQ(kExpected.size(), static_cast<size_t>(num_bytes_written));
   EXPECT_EQ(kExpected, base::StringPiece(output, num_bytes_written));
@@ skipping 1208 matching lines @@
   ASSERT_EQ(kBodySize, parser.ReadResponseBody(
       body_buffer.get(), kBodySize, callback.callback()));
 
   EXPECT_EQ(CountWriteBytes(writes, arraysize(writes)), parser.sent_bytes());
   EXPECT_EQ(CountReadBytes(reads, arraysize(reads)), parser.received_bytes());
 }
 
 }  // namespace
 
 }  // namespace net