Create blobs from Disk Cache entries.

net/base/upload_disk_cache_entry_element_reader_unittest.cc

+// Copyright 2015 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/base/upload_disk_cache_entry_element_reader.h"
+
+#include <algorithm>
+#include <string>
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/macros.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/time/time.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+#include "net/base/test_completion_callback.h"
+#include "net/disk_cache/disk_cache.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/platform_test.h"
+
+namespace net {
+namespace {
+
+const int kTestDiskCacheStreamIndex = 0;
+
+const char kDataKey[] = "a key";
+
+const char kData[] = "this is data in a disk cache entry";
+const size_t kDataSize = arraysize(kData) - 1;
+
+// A disk_cache::Entry that arbitrarily delays the completion of a
+// read operation to allow testing some races without flake.

[mmenke, 2015/06/16 18:51:19]

We're using an in-memory cache, which always returns synchronously, too - think
it's worth a comment here.

[gavinp, 2015/06/16 22:28:02]

Done.

+class DelayedReadEntry : public disk_cache::Entry {
+ public:
+  explicit DelayedReadEntry(disk_cache::ScopedEntryPtr entry)
+      : entry_(entry.Pass()) {}
+  ~DelayedReadEntry() override { EXPECT_FALSE(HasPendingReadCallbacks()); }
+
+  bool HasPendingReadCallbacks() { return !pending_read_callbacks_.empty(); }
+
+  void RunPendingReadCallbacks() {
+    for (const auto& callback : pending_read_callbacks_)
+      callback.Run();

[mmenke, 2015/06/16 18:51:19]

This seems scary.  Can we swap out the pending_read_callbacks_ before calling
them?

[gavinp, 2015/06/16 22:28:01]

Done.

+    pending_read_callbacks_.clear();
+  }
+
+  // From disk_cache::Entry:
+  void Doom() override { entry_->Doom(); }
+  void Close() override { delete this; }  // Note this is required by the API.
+  std::string GetKey() const override { return entry_->GetKey(); }
+  base::Time GetLastUsed() const override { return entry_->GetLastUsed(); }
+  base::Time GetLastModified() const override {
+    return entry_->GetLastModified();
+  }
+  int32 GetDataSize(int index) const override {
+    return entry_->GetDataSize(index);
+  }
+  int ReadData(int index,
+               int offset,
+               IOBuffer* buf,
+               int buf_len,
+               const CompletionCallback& original_callback) override {
+    TestCompletionCallback cb;

[mmenke, 2015/06/16 18:51:19]

Google style guide discourages abbreviations.  In my experience, callback is
generally written out.

[gavinp, 2015/06/16 22:28:02]

Done. Except in disk_cache code, which is where I learned the bad convention.
Thanks.

+    int rv = entry_->ReadData(index, offset, buf, buf_len, cb.callback());
+    rv = cb.GetResult(rv);

[mmenke, 2015/06/16 18:51:19]

The memory cache always returns synchronously, doesn't it?  Think it's clearer
if we have a comment on that (Could DCHECK on it, too, optionally)

[gavinp, 2015/06/16 22:28:02]

Done. Left the TestCompletionCallback in to avoid having a non-NULL callback
(per your request in the UploadDiskCacheEntryElementReader).

+    pending_read_callbacks_.push_back(base::Bind(original_callback, rv));
+    return ERR_IO_PENDING;
+  }
+  int WriteData(int index,
+                int offset,
+                IOBuffer* buf,
+                int buf_len,
+                const CompletionCallback& callback,
+                bool truncate) override {
+    return entry_->WriteData(index, offset, buf, buf_len, callback, truncate);
+  }
+  int ReadSparseData(int64 offset,
+                     IOBuffer* buf,
+                     int buf_len,
+                     const CompletionCallback& callback) override {
+    return entry_->ReadSparseData(offset, buf, buf_len, callback);
+  }
+  int WriteSparseData(int64 offset,
+                      IOBuffer* buf,
+                      int buf_len,
+                      const CompletionCallback& callback) override {
+    return entry_->WriteSparseData(offset, buf, buf_len, callback);
+  }
+  int GetAvailableRange(int64 offset,
+                        int len,
+                        int64* start,
+                        const CompletionCallback& callback) override {
+    return entry_->GetAvailableRange(offset, len, start, callback);
+  }
+  bool CouldBeSparse() const override { return entry_->CouldBeSparse(); }
+  void CancelSparseIO() override { entry_->CancelSparseIO(); }
+  int ReadyForSparseIO(const CompletionCallback& callback) override {
+    return ReadyForSparseIO(callback);
+  }
+
+ private:
+  disk_cache::ScopedEntryPtr entry_;
+  std::vector<base::Callback<void(void)>> pending_read_callbacks_;

[mmenke, 2015/06/16 18:51:19]

include <vector>

[gavinp, 2015/06/16 22:28:02]

Done.

+};
+
+class UploadDiskCacheEntryElementReaderTest : public PlatformTest {
+ public:
+  UploadDiskCacheEntryElementReaderTest() {}
+
+  ~UploadDiskCacheEntryElementReaderTest() override {}
+
+  void SetUp() override {
+    TestCompletionCallback cb;
+    int rv = disk_cache::CreateCacheBackend(MEMORY_CACHE, CACHE_BACKEND_DEFAULT,
+                                            base::FilePath(), 0, false, NULL,
+                                            NULL, &cache_, cb.callback());
+    ASSERT_EQ(OK, cb.GetResult(rv));
+
+    disk_cache::Entry* tmp_entry = nullptr;
+    rv = cache_->CreateEntry(kDataKey, &tmp_entry, cb.callback());
+    ASSERT_EQ(OK, cb.GetResult(rv));
+    entry_.reset(tmp_entry);
+
+    scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(kData);
+    rv = entry_->WriteData(kTestDiskCacheStreamIndex, 0, iobuffer.get(),
+                           kDataSize, cb.callback(), false);
+    EXPECT_EQ(static_cast<int>(kDataSize), cb.GetResult(rv));
+  }
+
+ protected:

[mmenke, 2015/06/16 18:51:19]

protected not needed in test fixtures

[gavinp, 2015/06/16 22:28:02]

Done.

+  void set_entry(disk_cache::ScopedEntryPtr entry) { entry_.swap(entry); }
+  disk_cache::Entry* entry() { return entry_.get(); }
+  disk_cache::ScopedEntryPtr release_entry() { return entry_.Pass(); }
+
+ private:
+  scoped_ptr<disk_cache::Backend> cache_;
+  disk_cache::ScopedEntryPtr entry_;
+};
+
+TEST_F(UploadDiskCacheEntryElementReaderTest, ReadAll) {
+  UploadDiskCacheEntryElementReader reader(entry(), kTestDiskCacheStreamIndex,
+                                           0, kDataSize);
+  EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining());
+
+  char read_buffer[kDataSize];
+  std::fill(read_buffer, read_buffer + arraysize(read_buffer), '\0');
+
+  scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(read_buffer);

[mmenke, 2015/06/16 18:51:19]

include ref_counted

[mmenke, 2015/06/16 18:51:19]

nit:  io_buffer

[gavinp, 2015/06/16 22:28:02]

Done.

[gavinp, 2015/06/16 22:28:02]

Done.

+  TestCompletionCallback cb;
+  int rv = reader.Read(iobuffer.get(), kDataSize, cb.callback());
+  EXPECT_EQ(static_cast<int>(kDataSize), cb.GetResult(rv));
+  EXPECT_EQ(0U, reader.BytesRemaining());

[mmenke, 2015/06/16 18:51:19]

We're relying on the underlying cache to handle reads of |kDataSize| in one go. 
Think that's worth commenting on.

[gavinp, 2015/06/16 22:28:02]

Done.

+  EXPECT_TRUE(std::equal(kData, kData + kDataSize, read_buffer));

[mmenke, 2015/06/16 18:51:19]

Suggest comparing them as strings using EXPECT_EQ, to get better error output on
failure.

[gavinp, 2015/06/16 22:28:01]

Done.

+}
+
+TEST_F(UploadDiskCacheEntryElementReaderTest, ReadPartially) {
+  UploadDiskCacheEntryElementReader reader(entry(), kTestDiskCacheStreamIndex,
+                                           0, kDataSize);
+  EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining());

[mmenke, 2015/06/16 18:51:19]

uint64_t (Not going to mark the rest of these)

[gavinp, 2015/06/16 22:28:02]

Done.

+
+  char read_buffer[kDataSize];
+  std::fill(read_buffer, read_buffer + arraysize(read_buffer), '\0');
+
+  const size_t iobuffer1_size = kDataSize / 3;

[mmenke, 2015/06/16 18:51:19]

Think these should kConstantNaming

[gavinp, 2015/06/16 22:28:02]

Done.

+  scoped_refptr<IOBuffer> iobuffer1 = new WrappedIOBuffer(read_buffer);
+
+  const size_t iobuffer2_size = kDataSize - iobuffer1_size;

[mmenke, 2015/06/16 18:51:19]

Out of paranoia, suggest separate buffers, with no extra space.  Otherwise could
imagine a lot of weird behaviors that would pass this test, even in debug
builds.

[gavinp, 2015/06/16 22:28:02]

Done. But I don't know what you meant by "no extra space", I ended up just using
full kDataSize buffers, because why not? Was that contrary to your intention?

+  scoped_refptr<IOBuffer> iobuffer2 =
+      new WrappedIOBuffer(read_buffer + iobuffer1_size);
+
+  TestCompletionCallback cb;
+  int rv = reader.Read(iobuffer1.get(), iobuffer1_size, cb.callback());
+  EXPECT_EQ(static_cast<int>(iobuffer1_size), cb.GetResult(rv));
+  EXPECT_EQ(static_cast<uint64>(iobuffer2_size), reader.BytesRemaining());

[mmenke, 2015/06/16 18:51:19]

uint64_t?  And include <stdint>

[gavinp, 2015/06/16 22:28:02]

Done. Went with <cstdint> and so std::size_t everywhere.

[gavinp, 2015/06/16 22:47:32]

Switched back to <stdint.h>, see the discussion in
https://chromium-cpp.appspot.com/ . C++11 library features are not allowed, so
that includes <cstdint>. As well, <stdint.h> is explicitly permitted. Not to
mention some builders don't have <cstdint>.

+
+  rv = reader.Read(iobuffer2.get(), iobuffer2_size, cb.callback());
+  EXPECT_EQ(static_cast<int>(iobuffer2_size), cb.GetResult(rv));
+  EXPECT_EQ(0U, reader.BytesRemaining());
+
+  EXPECT_TRUE(std::equal(kData, kData + kDataSize, read_buffer));
+}
+
+TEST_F(UploadDiskCacheEntryElementReaderTest, ReadTooMuch) {
+  UploadDiskCacheEntryElementReader reader(entry(), kTestDiskCacheStreamIndex,
+                                           0, kDataSize);
+  EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining());
+
+  const size_t kTooLargeSize = kDataSize + kDataSize / 2;
+
+  char read_buffer[kTooLargeSize];
+  std::fill(read_buffer, read_buffer + arraysize(read_buffer), '\0');
+
+  scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(read_buffer);
+  TestCompletionCallback cb;
+  int rv = reader.Read(iobuffer.get(), kTooLargeSize, cb.callback());
+  EXPECT_EQ(static_cast<int>(kDataSize), cb.GetResult(rv));
+  EXPECT_EQ(0U, reader.BytesRemaining());
+  EXPECT_TRUE(std::equal(kData, kData + kDataSize, read_buffer));
+}
+
+TEST_F(UploadDiskCacheEntryElementReaderTest, MultipleInit) {
+  UploadDiskCacheEntryElementReader reader(entry(), kTestDiskCacheStreamIndex,
+                                           0, kDataSize);
+  char read_buffer[kDataSize];
+  std::fill(read_buffer, read_buffer + arraysize(read_buffer), '\0');
+
+  scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(read_buffer);
+  TestCompletionCallback cb;
+  int rv = reader.Read(iobuffer.get(), kDataSize, cb.callback());
+  EXPECT_EQ(static_cast<int>(kDataSize), cb.GetResult(rv));
+  EXPECT_TRUE(std::equal(kData, kData + kDataSize, read_buffer));
+
+  rv = reader.Init(cb.callback());
+  EXPECT_EQ(OK, cb.GetResult(rv));
+  EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining());
+  rv = reader.Read(iobuffer.get(), kDataSize, cb.callback());
+  EXPECT_EQ(static_cast<int>(kDataSize), cb.GetResult(rv));
+  EXPECT_TRUE(std::equal(kData, kData + kDataSize, read_buffer));
+}

[mmenke, 2015/06/16 18:51:19]

Should have a generic async test (Per earlier comment, these all complete
synchronously)

[gavinp, 2015/06/16 22:28:02]

Done.

More problematically, UploadFileElementReader needs a sync test; it has a
serious bug that it calls the callback _and_ returns a value when a read on a
file completes synchronously. Amazingly, there's a comment in that file
documenting the possibility of a file returning synchronously from a read just a
few lines above the code that ultimately bugs.

+
+TEST_F(UploadDiskCacheEntryElementReaderTest, InitDuringAsyncOperation) {
+  DelayedReadEntry* delayed_read_entry = new DelayedReadEntry(release_entry());
+  set_entry(disk_cache::ScopedEntryPtr(delayed_read_entry));
+
+  UploadDiskCacheEntryElementReader reader(entry(), kTestDiskCacheStreamIndex,
+                                           0, kDataSize);
+  char read_buffer[kDataSize];
+  std::fill(read_buffer, read_buffer + arraysize(read_buffer), '\0');
+
+  scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(read_buffer);
+  int rv = reader.Read(iobuffer.get(), kDataSize, CompletionCallback());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  EXPECT_TRUE(delayed_read_entry->HasPendingReadCallbacks());
+  EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining());
+
+  TestCompletionCallback cb;
+  rv = reader.Init(cb.callback());
+  EXPECT_EQ(OK, cb.GetResult(rv));
+
+  delayed_read_entry->RunPendingReadCallbacks();
+  EXPECT_EQ(static_cast<uint64>(kDataSize), reader.BytesRemaining());

[mmenke, 2015/06/16 18:51:19]

Should do a second read here (To a different buffer), and let it complete.

[gavinp, 2015/06/16 22:28:02]

Done.

+}
+
+TEST_F(UploadDiskCacheEntryElementReaderTest, Range) {
+  const size_t kOffset = kDataSize / 4;
+  const size_t kLength = kDataSize / 3;
+
+  UploadDiskCacheEntryElementReader reader(entry(), kTestDiskCacheStreamIndex,
+                                           kOffset, kLength);
+  EXPECT_EQ(static_cast<uint64>(kLength), reader.BytesRemaining());
+
+  char read_buffer[kLength];
+  std::fill(read_buffer, read_buffer + arraysize(read_buffer), '\0');
+
+  scoped_refptr<IOBuffer> iobuffer = new WrappedIOBuffer(read_buffer);
+  TestCompletionCallback cb;
+  int rv = reader.Read(iobuffer.get(), kLength, cb.callback());
+  EXPECT_EQ(static_cast<int>(kLength), cb.GetResult(rv));
+  EXPECT_EQ(0U, reader.BytesRemaining());
+  EXPECT_TRUE(
+      std::equal(kData + kOffset, kData + kOffset + kLength, read_buffer));
+}
+
+}  // namespace
+}  // namespace net