From 7b822b2be3e9eff10ffb9065f2449678fc0aab59 Mon Sep 17 00:00:00 2001 From: "wtc@google.com" Date: Tue, 5 Aug 2008 00:15:45 +0000 Subject: Miscellaneous changes (mostly cleanup) from my code review. R=darin@google.com git-svn-id: svn://svn.chromium.org/chrome/trunk/src@354 0039d316-1c4b-4281-b951-d872f2087c98 --- net/base/ssl_client_socket.h | 4 ++ net/base/ssl_client_socket_unittest.cc | 58 ++++++++++++++++------------ net/http/http_chunked_decoder_unittest.cc | 9 ++++- net/http/http_connection.h | 13 ++++--- net/http/http_connection_manager.cc | 39 ++++++++++++------- net/http/http_connection_manager.h | 16 ++++---- net/http/http_connection_manager_unittest.cc | 6 --- net/http/http_network_layer.h | 6 +-- 8 files changed, 90 insertions(+), 61 deletions(-) (limited to 'net') diff --git a/net/base/ssl_client_socket.h b/net/base/ssl_client_socket.h index 599b488..3e1779f 100644 --- a/net/base/ssl_client_socket.h +++ b/net/base/ssl_client_socket.h @@ -35,12 +35,16 @@ #include #include +#include + #include "base/scoped_ptr.h" #include "net/base/client_socket.h" #include "net/base/completion_callback.h" namespace net { +// A client socket that uses SSL as the transport layer. +// // NOTE: The SSL handshake occurs within the Connect method after a TCP // connection is established. If a SSL error occurs during the handshake, // Connect will fail. The consumer may choose to ignore certain SSL errors, diff --git a/net/base/ssl_client_socket_unittest.cc b/net/base/ssl_client_socket_unittest.cc index a465563..ebcce65 100644 --- a/net/base/ssl_client_socket_unittest.cc +++ b/net/base/ssl_client_socket_unittest.cc @@ -28,8 +28,8 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. #include "net/base/address_list.h" -#include "net/base/net_errors.h" #include "net/base/host_resolver.h" +#include "net/base/net_errors.h" #include "net/base/ssl_client_socket.h" #include "net/base/tcp_client_socket.h" #include "net/base/test_completion_callback.h" @@ -60,10 +60,12 @@ TEST_F(SSLClientSocketTest, Connect) { EXPECT_FALSE(sock.IsConnected()); rv = sock.Connect(&callback); - ASSERT_EQ(net::ERR_IO_PENDING, rv); + if (rv != net::OK) { + ASSERT_EQ(net::ERR_IO_PENDING, rv); - rv = callback.WaitForResult(); - EXPECT_EQ(net::OK, rv); + rv = callback.WaitForResult(); + EXPECT_EQ(net::OK, rv); + } EXPECT_TRUE(sock.IsConnected()); @@ -87,18 +89,20 @@ TEST_F(SSLClientSocketTest, Read) { net::SSLClientSocket sock(new net::TCPClientSocket(addr), hostname); rv = sock.Connect(&callback); - ASSERT_EQ(rv, net::ERR_IO_PENDING); + if (rv != net::OK) { + ASSERT_EQ(rv, net::ERR_IO_PENDING); - rv = callback.WaitForResult(); - EXPECT_EQ(rv, net::OK); + rv = callback.WaitForResult(); + EXPECT_EQ(rv, net::OK); + } const char request_text[] = "GET / HTTP/1.0\r\n\r\n"; - rv = sock.Write(request_text, arraysize(request_text)-1, &callback); + rv = sock.Write(request_text, arraysize(request_text) - 1, &callback); EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING); if (rv == net::ERR_IO_PENDING) { rv = callback.WaitForResult(); - EXPECT_EQ(rv, arraysize(request_text)-1); + EXPECT_EQ(rv, arraysize(request_text) - 1); } char buf[4096]; @@ -109,12 +113,13 @@ TEST_F(SSLClientSocketTest, Read) { if (rv == net::ERR_IO_PENDING) rv = callback.WaitForResult(); - if (rv == 0) + EXPECT_GE(rv, 0); + if (rv <= 0) break; } } -TEST_F(TCPClientSocketTest, Read_SmallChunks) { +TEST_F(SSLClientSocketTest, Read_SmallChunks) { net::AddressList addr; net::HostResolver resolver; TestCompletionCallback callback; @@ -125,18 +130,20 @@ TEST_F(TCPClientSocketTest, Read_SmallChunks) { net::TCPClientSocket sock(addr); rv = sock.Connect(&callback); - ASSERT_EQ(rv, net::ERR_IO_PENDING); + if (rv != net::OK) { + ASSERT_EQ(rv, net::ERR_IO_PENDING); - rv = callback.WaitForResult(); - EXPECT_EQ(rv, net::OK); + rv = callback.WaitForResult(); + EXPECT_EQ(rv, net::OK); + } const char request_text[] = "GET / HTTP/1.0\r\n\r\n"; - rv = sock.Write(request_text, arraysize(request_text)-1, &callback); + rv = sock.Write(request_text, arraysize(request_text) - 1, &callback); EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING); if (rv == net::ERR_IO_PENDING) { rv = callback.WaitForResult(); - EXPECT_EQ(rv, arraysize(request_text)-1); + EXPECT_EQ(rv, arraysize(request_text) - 1); } char buf[1]; @@ -147,12 +154,13 @@ TEST_F(TCPClientSocketTest, Read_SmallChunks) { if (rv == net::ERR_IO_PENDING) rv = callback.WaitForResult(); - if (rv == 0) + EXPECT_GE(rv, 0); + if (rv <= 0) break; } } -TEST_F(TCPClientSocketTest, Read_Interrupted) { +TEST_F(SSLClientSocketTest, Read_Interrupted) { net::AddressList addr; net::HostResolver resolver; TestCompletionCallback callback; @@ -163,18 +171,20 @@ TEST_F(TCPClientSocketTest, Read_Interrupted) { net::TCPClientSocket sock(addr); rv = sock.Connect(&callback); - ASSERT_EQ(rv, net::ERR_IO_PENDING); + if (rv != net::OK) { + ASSERT_EQ(rv, net::ERR_IO_PENDING); - rv = callback.WaitForResult(); - EXPECT_EQ(rv, net::OK); + rv = callback.WaitForResult(); + EXPECT_EQ(rv, net::OK); + } const char request_text[] = "GET / HTTP/1.0\r\n\r\n"; - rv = sock.Write(request_text, arraysize(request_text)-1, &callback); + rv = sock.Write(request_text, arraysize(request_text) - 1, &callback); EXPECT_TRUE(rv >= 0 || rv == net::ERR_IO_PENDING); if (rv == net::ERR_IO_PENDING) { rv = callback.WaitForResult(); - EXPECT_EQ(rv, arraysize(request_text)-1); + EXPECT_EQ(rv, arraysize(request_text) - 1); } // Do a partial read and then exit. This test should not crash! @@ -185,6 +195,6 @@ TEST_F(TCPClientSocketTest, Read_Interrupted) { if (rv == net::ERR_IO_PENDING) rv = callback.WaitForResult(); - EXPECT_TRUE(rv != 0); + EXPECT_NE(rv, 0); } #endif diff --git a/net/http/http_chunked_decoder_unittest.cc b/net/http/http_chunked_decoder_unittest.cc index 602ba6a..377f1b1 100644 --- a/net/http/http_chunked_decoder_unittest.cc +++ b/net/http/http_chunked_decoder_unittest.cc @@ -46,7 +46,7 @@ void RunTest(const char* inputs[], size_t num_inputs, for (size_t i = 0; i < num_inputs; ++i) { std::string input = inputs[i]; int n = decoder.FilterBuf(&input[0], static_cast(input.size())); - EXPECT_TRUE(n >= 0); + EXPECT_GE(n, 0); if (n > 0) result.append(input.data(), n); } @@ -64,6 +64,13 @@ TEST(HttpChunkedDecoderTest, Basic) { RunTest(inputs, arraysize(inputs), "hello", true); } +TEST(HttpChunkedDecoderTest, OneChunk) { + const char* inputs[] = { + "5\r\nhello\r\n" + }; + RunTest(inputs, arraysize(inputs), "hello", false); +} + TEST(HttpChunkedDecoderTest, Typical) { const char* inputs[] = { "5\r\nhello\r\n", diff --git a/net/http/http_connection.h b/net/http/http_connection.h index ada8468..e3a6496 100644 --- a/net/http/http_connection.h +++ b/net/http/http_connection.h @@ -30,6 +30,8 @@ #ifndef NET_HTTP_HTTP_CONNECTION_H_ #define NET_HTTP_HTTP_CONNECTION_H_ +#include + #include "base/ref_counted.h" #include "base/scoped_ptr.h" #include "net/base/client_socket.h" @@ -50,11 +52,12 @@ class HttpConnectionManager; // class HttpConnection { public: - HttpConnection(HttpConnectionManager* mgr); + explicit HttpConnection(HttpConnectionManager* mgr); ~HttpConnection(); // Initializes a HttpConnection object, which involves talking to the - // HttpConnectionManager to locate a socket to possibly reuse. + // HttpConnectionManager to locate a socket to possibly reuse. This method + // returns either OK or ERR_IO_PENDING. // // If this method succeeds, then the socket member will be set to an existing // socket if an existing socket was available to reuse. Otherwise, the @@ -69,9 +72,9 @@ class HttpConnection { // An initialized connection can be reset, which causes it to return to the // un-initialized state. This releases the underlying socket, which in the - // case of a socket that is not closed, indicates that the socket may be kept - // alive for use by a subsequent HttpConnection. NOTE: To prevent the socket - // from being kept alive, be sure to call its Close method. + // case of a socket that is not disconnected, indicates that the socket may + // be kept alive for use by a subsequent HttpConnection. NOTE: To prevent + // the socket from being kept alive, be sure to call its Disconnect method. void Reset(); // Returns true when Init has completed successfully. diff --git a/net/http/http_connection_manager.cc b/net/http/http_connection_manager.cc index a7edd92..7064c4f 100644 --- a/net/http/http_connection_manager.cc +++ b/net/http/http_connection_manager.cc @@ -33,18 +33,25 @@ #include "net/base/client_socket.h" #include "net/base/net_errors.h" +namespace { + +// The timeout value, in seconds, used to clean up disconnected idle sockets. +const int kCleanupInterval = 5; + +} // namespace + namespace net { HttpConnectionManager::HttpConnectionManager() - : timer_(TimeDelta::FromSeconds(5)), - idle_count_(0) { + : timer_(TimeDelta::FromSeconds(kCleanupInterval)), + idle_socket_count_(0) { timer_.set_task(this); } HttpConnectionManager::~HttpConnectionManager() { timer_.set_task(NULL); - // Cleanup any idle sockets. Assert that we have no remaining active sockets + // Clean up any idle sockets. Assert that we have no remaining active sockets // or pending requests. They should have all been cleaned up prior to the // manager being destroyed. @@ -55,13 +62,13 @@ HttpConnectionManager::~HttpConnectionManager() { int HttpConnectionManager::RequestSocket(const std::string& group_name, SocketHandle** handle, CompletionCallback* callback) { - Group& group = - group_map_.insert(std::make_pair(group_name, Group())).first->second; + Group& group = group_map_[group_name]; // Can we make another active socket now? if (group.active_socket_count == kMaxSocketsPerGroup) { Request r; r.result = handle; + DCHECK(callback); r.callback = callback; group.pending_requests.push_back(r); return ERR_IO_PENDING; @@ -70,18 +77,18 @@ int HttpConnectionManager::RequestSocket(const std::string& group_name, // OK, we are going to activate one. group.active_socket_count++; - // Use idle sockets in LIFO order. + // Use idle sockets in LIFO order because they're more likely to be + // still connected. while (!group.idle_sockets.empty()) { SocketHandle* h = group.idle_sockets.back(); group.idle_sockets.pop_back(); DecrementIdleCount(); - if (!h->get()->IsConnected()) { - delete h; - } else { + if (h->get()->IsConnected()) { // We found one we can reuse! *handle = h; return OK; } + delete h; } *handle = new SocketHandle(); @@ -93,7 +100,8 @@ void HttpConnectionManager::CancelRequest(const std::string& group_name, Group& group = group_map_[group_name]; // In order for us to be canceling a pending request, we must have active - // sockets equaling the limit. + // sockets equaling the limit. NOTE: The correctness of the code doesn't + // require this assertion. DCHECK(group.active_socket_count == kMaxSocketsPerGroup); // Search pending_requests for matching handle. @@ -120,7 +128,7 @@ void HttpConnectionManager::CloseIdleSockets() { void HttpConnectionManager::MaybeCloseIdleSockets( bool only_if_disconnected) { - if (idle_count_ == 0) + if (idle_socket_count_ == 0) return; GroupMap::iterator i = group_map_.begin(); @@ -149,12 +157,12 @@ void HttpConnectionManager::MaybeCloseIdleSockets( } void HttpConnectionManager::IncrementIdleCount() { - if (++idle_count_ == 1) + if (++idle_socket_count_ == 1) timer_.Start(); } void HttpConnectionManager::DecrementIdleCount() { - if (--idle_count_ == 0) + if (--idle_socket_count_ == 0) timer_.Stop(); } @@ -180,8 +188,9 @@ void HttpConnectionManager::DoReleaseSocket(const std::string& group_name, if (!group.pending_requests.empty()) { Request r = group.pending_requests.front(); group.pending_requests.pop_front(); - RequestSocket(i->first, r.result, NULL); - r.callback->Run(OK); + int rv = RequestSocket(i->first, r.result, NULL); + DCHECK(rv == OK); + r.callback->Run(rv); return; } diff --git a/net/http/http_connection_manager.h b/net/http/http_connection_manager.h index 22080a3..fc5402c 100644 --- a/net/http/http_connection_manager.h +++ b/net/http/http_connection_manager.h @@ -32,6 +32,7 @@ #include #include +#include #include "base/ref_counted.h" #include "base/timer.h" @@ -45,7 +46,7 @@ class ClientSocket; // open at a time. It also maintains a list of idle persistent sockets. // // The HttpConnectionManager allocates SocketHandle objects, but it is not -// responsible for allocating the associated ClientSocket object. The +// responsible for allocating the associated ClientSocket objects. The // consumer must do so if it gets a SocketHandle with a null ClientSocket. // class HttpConnectionManager : public base::RefCounted, @@ -62,11 +63,10 @@ class HttpConnectionManager : public base::RefCounted, // // If this function returns OK, then |*handle| will reference a SocketHandle // object. If ERR_IO_PENDING is returned, then the completion callback will - // be called when |*handle| has been populated. Otherwise, an error code is - // returned. + // be called when |*handle| has been populated. // // If the resultant SocketHandle object has a null member, then it is the - // callers job to create a ClientSocket and associate it with the handle. + // caller's job to create a ClientSocket and associate it with the handle. // int RequestSocket(const std::string& group_name, SocketHandle** handle, CompletionCallback* callback); @@ -91,7 +91,9 @@ class HttpConnectionManager : public base::RefCounted, ~HttpConnectionManager(); // Closes all idle sockets if |only_if_disconnected| is false. Else, only - // idle sockets that are disconnected get closed. + // idle sockets that are disconnected get closed. "Maybe" refers to the + // fact that it doesn't close all idle sockets if |only_if_disconnected| is + // true. void MaybeCloseIdleSockets(bool only_if_disconnected); // Called when the number of idle sockets changes. @@ -115,10 +117,10 @@ class HttpConnectionManager : public base::RefCounted, // A Group is allocated per group_name when there are idle sockets or pending // requests. Otherwise, the Group object is removed from the map. struct Group { + Group() : active_socket_count(0) {} std::deque idle_sockets; std::deque pending_requests; int active_socket_count; - Group() : active_socket_count(0) {} }; typedef std::map GroupMap; @@ -128,7 +130,7 @@ class HttpConnectionManager : public base::RefCounted, RepeatingTimer timer_; // The total number of idle sockets in the system. - int idle_count_; + int idle_socket_count_; }; } // namespace net diff --git a/net/http/http_connection_manager_unittest.cc b/net/http/http_connection_manager_unittest.cc index 9eade18..503097b 100644 --- a/net/http/http_connection_manager_unittest.cc +++ b/net/http/http_connection_manager_unittest.cc @@ -67,12 +67,6 @@ class MockClientSocket : public net::ClientSocket { net::CompletionCallback* callback) { return net::ERR_FAILED; } - virtual int GetProperty(int property, void* buf, int buf_len) const { - return net::ERR_FAILED; - } - virtual int SetProperty(int property, const void* buf, int buf_len) { - return net::ERR_FAILED; - } static int allocation_count; diff --git a/net/http/http_network_layer.h b/net/http/http_network_layer.h index 72fbca3..db7d0d9 100644 --- a/net/http/http_network_layer.h +++ b/net/http/http_network_layer.h @@ -40,6 +40,9 @@ class HttpProxyInfo; class HttpNetworkLayer : public HttpTransactionFactory { public: + explicit HttpNetworkLayer(const HttpProxyInfo* pi); + ~HttpNetworkLayer(); + // This function hides the details of how a network layer gets instantiated // and allows other implementations to be substituted. static HttpTransactionFactory* CreateFactory(const HttpProxyInfo* pi); @@ -47,9 +50,6 @@ class HttpNetworkLayer : public HttpTransactionFactory { // If value is true, then WinHTTP will be used. static void UseWinHttp(bool value); - HttpNetworkLayer(const HttpProxyInfo* pi); - ~HttpNetworkLayer(); - // HttpTransactionFactory methods: virtual HttpTransaction* CreateTransaction(); virtual HttpCache* GetCache(); -- cgit v1.1