Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10878090:
Keep pool of pre-connected DNS sockets (Closed)
|
Created:
8 years, 3 months ago by Deprecated (see juliatuttle) Modified:
8 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionKeep pool of pre-connected DNS sockets.
This adds more entropy to our source ports on Windows, which allocates ports sequentially. (We explicitly pick random ports on other platforms, but asking for a particular source port triggers a firewall warning on Windows, so we don't.)
BUG=107413
TEST=DnsTransactionTest
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=173038
Patch Set 1 #
Total comments: 9
Patch Set 2 : Move to socket buffers and callbacks for dependency injection #
Total comments: 12
Patch Set 3 : Use a single, separate DnsSocketPool object #
Total comments: 80
Patch Set 4 : #
Total comments: 48
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 1
Patch Set 7 : rebase #Patch Set 8 : Fix DnsSession destructor #Patch Set 9 : NET_EXPORT_PRIVATE DnsSocketPool #Patch Set 10 : Don't OVERRIDE declarations #Patch Set 11 : Avoid tautological comparison warning from clang #Patch Set 12 : rebase #
Total comments: 1
Patch Set 13 : Add comments to dns_socket_pool.h #Patch Set 14 : rebase #Patch Set 15 : Make SocketLease NET_EXPORT_PRIVATE explciitly #Patch Set 16 : rebase #
Total comments: 1
Patch Set 17 : Add second NET_EXPORT_PRIVATE back #
Messages
Total messages: 40 (0 generated)
Could this just use the ClientSocketHandle/ClientSocketPool classes instead of creating something very similar? http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.cc File net/dns/dns_session.cc (right): http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.cc#newcode102 net/dns/dns_session.cc:102: socket.reset(NULL); Is this always guaranteed to complete synchronously? http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h File net/dns/dns_session.h (right): http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h#newcode66 net/dns/dns_session.h:66: scoped_ptr<SocketLease> AllocateSocket(int server_index); Not sure that I like using a server_index for this - what happens when the config changes and the servers differ? Should this be an IPEndpoint instead? http://codereview.chromium.org/10878090/diff/1/net/dns/dns_transaction.cc File net/dns/dns_transaction.cc (right): http://codereview.chromium.org/10878090/diff/1/net/dns/dns_transaction.cc#new... net/dns/dns_transaction.cc:397: net_log_.AddEvent( Why did this move?
On 2012/08/29 16:48:06, cbentzel wrote: > Could this just use the ClientSocketHandle/ClientSocketPool classes instead of > creating something very similar? Note: It's possible that ClientSocketHandle and ClientSocketPool are not appropriate here, or too complex - but I'd like you to see whether they are applicable. Regardless, I think I'd like to have Pool/Handle like classes for this which are separate from DnsSession, but pass the Pool in instead. > > http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.cc > File net/dns/dns_session.cc (right): > > http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.cc#newcode102 > net/dns/dns_session.cc:102: socket.reset(NULL); > Is this always guaranteed to complete synchronously? > > http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h > File net/dns/dns_session.h (right): > > http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h#newcode66 > net/dns/dns_session.h:66: scoped_ptr<SocketLease> AllocateSocket(int > server_index); > Not sure that I like using a server_index for this - what happens when the > config changes and the servers differ? Should this be an IPEndpoint instead? > > http://codereview.chromium.org/10878090/diff/1/net/dns/dns_transaction.cc > File net/dns/dns_transaction.cc (right): > > http://codereview.chromium.org/10878090/diff/1/net/dns/dns_transaction.cc#new... > net/dns/dns_transaction.cc:397: net_log_.AddEvent( > Why did this move?
http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.cc File net/dns/dns_session.cc (right): http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.cc#newcode102 net/dns/dns_session.cc:102: socket.reset(NULL); On 2012/08/29 16:48:06, cbentzel wrote: > Is this always guaranteed to complete synchronously? Connect? Yes. Note that it does not take a CompletionCallback argument. http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h File net/dns/dns_session.h (right): http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h#newcode66 net/dns/dns_session.h:66: scoped_ptr<SocketLease> AllocateSocket(int server_index); On 2012/08/29 16:48:06, cbentzel wrote: > Not sure that I like using a server_index for this - what happens when the > config changes and the servers differ? Should this be an IPEndpoint instead? If it's an IPEndPoint, this will have to keep a map, which seems overkill. DnsSession is fixed to DnsConfig (note consts). If DnsConfig changes, you will need to create a new DnsSession. http://codereview.chromium.org/10878090/diff/1/net/dns/dns_transaction.cc File net/dns/dns_transaction.cc (right): http://codereview.chromium.org/10878090/diff/1/net/dns/dns_transaction.cc#new... net/dns/dns_transaction.cc:397: net_log_.AddEvent( On 2012/08/29 16:48:06, cbentzel wrote: > Why did this move? In my draft CL, this moved because it needs a reference to the socket which |attempt| now owns. I thought it's not worth the complexity to first obtain the socket, log the event, then pass it to the attempt.
http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h File net/dns/dns_session.h (right): http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h#newcode66 net/dns/dns_session.h:66: scoped_ptr<SocketLease> AllocateSocket(int server_index); On 2012/08/29 16:57:08, szym wrote: > On 2012/08/29 16:48:06, cbentzel wrote: > > Not sure that I like using a server_index for this - what happens when the > > config changes and the servers differ? Should this be an IPEndpoint instead? > > If it's an IPEndPoint, this will have to keep a map, which seems overkill. > DnsSession is fixed to DnsConfig (note consts). If DnsConfig changes, you will > need to create a new DnsSession. OK, int may be fine. But would we want to be able to reuse sockets across DnsConfig changes?
https://chromiumcodereview.appspot.com/10878090/diff/1/net/dns/dns_session.h File net/dns/dns_session.h (right): https://chromiumcodereview.appspot.com/10878090/diff/1/net/dns/dns_session.h#... net/dns/dns_session.h:66: scoped_ptr<SocketLease> AllocateSocket(int server_index); On 2012/08/29 17:03:48, cbentzel wrote: > On 2012/08/29 16:57:08, szym wrote: > > On 2012/08/29 16:48:06, cbentzel wrote: > > > Not sure that I like using a server_index for this - what happens when the > > > config changes and the servers differ? Should this be an IPEndpoint instead? > > > > If it's an IPEndPoint, this will have to keep a map, which seems overkill. > > DnsSession is fixed to DnsConfig (note consts). If DnsConfig changes, you will > > need to create a new DnsSession. > > OK, int may be fine. But would we want to be able to reuse sockets across > DnsConfig changes? I don't think DnsConfig changes are that common that it would warrant the complexity? I could be convinced, though.
Do we gain anything from re-using sockets, rather than just creating new ones when we remove old ones from the pool?
On 2012/08/29 17:57:35, Matt Menke wrote: > Do we gain anything from re-using sockets, rather than just creating new ones > when we remove old ones from the pool? The original motivation is that, on Windows, we can't pick random source ports, so we have to use whatever the OS gives us, which can be (is?) sequential, which makes DNS spoofing attacks easier. The pool would let us pick, from, say, 1024 of the not-random ports the OS has given us, making it at least a little harder.
On 2012/08/29 18:14:59, ttuttle wrote: > On 2012/08/29 17:57:35, Matt Menke wrote: > > Do we gain anything from re-using sockets, rather than just creating new ones > > when we remove old ones from the pool? > > The original motivation is that, on Windows, we can't pick random source ports, > so we have to use whatever the OS gives us, which can be (is?) sequential, which > makes DNS spoofing attacks easier. The pool would let us pick, from, say, 1024 > of the not-random ports the OS has given us, making it at least a little harder. Right. But if we had a pool of 100 sockets, picked a random one to use, and then instead of adding the picked one back, just created a new one, we'd achieve the same thing. Shouldn't have any real impact on security, one way or another. Just thinking more in terms of keeping the interface simple.
On 2012/08/29 18:16:46, Matt Menke wrote: > On 2012/08/29 18:14:59, ttuttle wrote: > > On 2012/08/29 17:57:35, Matt Menke wrote: > > > Do we gain anything from re-using sockets, rather than just creating new > ones > > > when we remove old ones from the pool? > > > > The original motivation is that, on Windows, we can't pick random source > ports, > > so we have to use whatever the OS gives us, which can be (is?) sequential, > which > > makes DNS spoofing attacks easier. The pool would let us pick, from, say, > 1024 > > of the not-random ports the OS has given us, making it at least a little > harder. > > Right. But if we had a pool of 100 sockets, picked a random one to use, and > then instead of adding the picked one back, just created a new one, we'd achieve > the same thing. > > Shouldn't have any real impact on security, one way or another. Just thinking > more in terms of keeping the interface simple. Oh, that's clever -- more like a buffer than a pool (you never return them). I would happily change this to work that way, if that's what people want.
Oh, one reason to do the pool solution is that, whether it's using Chrome-chosen or OS-assigned ports, it also helps with NAT congestion by limiting our total 'port footprint'.
http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h File net/dns/dns_session.h (right): http://codereview.chromium.org/10878090/diff/1/net/dns/dns_session.h#newcode49 net/dns/dns_session.h:49: bool bypass_pools, You could also consider making SocketPool a separate interface+class and passing an instance here. This is a more common approach to disabling some behaviors in tests.
I just went to do that, and here's the sticky part: If we want to, say, preallocate 256 sockets when we create the buffer/pool/whatever, then it needs a way of allocating sockets, not just putting and getting ones allocated elsewhere. This means we're passing it some sort of factory, and what was a couple of functions, some vectors, and an extra constructor turns into something like six classes (SocketFactory, SocketPool, SocketPoolFactory, SocketFactoryImpl, SocketPoolImpl, SocketPoolImplFactory). If this complexity is okay, I'll go for it, but it seems excessive.
On 2012/09/05 01:33:15, ttuttle wrote: > I just went to do that, and here's the sticky part: If we want to, say, > preallocate 256 sockets when we create the buffer/pool/whatever, then it needs a > way of allocating sockets, not just putting and getting ones allocated > elsewhere. This means we're passing it some sort of factory, and what was a > couple of functions, some vectors, and an extra constructor turns into something > like six classes (SocketFactory, SocketPool, SocketPoolFactory, > SocketFactoryImpl, SocketPoolImpl, SocketPoolImplFactory). If this complexity > is okay, I'll go for it, but it seems excessive. Could you use lazy allocation? That is, don't pre-allocate sockets, but if the current pool is less than N, then just allocate a new one. I think SocketPool + SocketPoolImpl should suffice. And I'd even be okay if SocketPoolImpl was hidden in an unnamed namespace inside socket_pool.cc
On 2012/09/05 02:43:33, szym wrote: > On 2012/09/05 01:33:15, ttuttle wrote: > > I just went to do that, and here's the sticky part: If we want to, say, > > preallocate 256 sockets when we create the buffer/pool/whatever, then it needs > a > > way of allocating sockets, not just putting and getting ones allocated > > elsewhere. This means we're passing it some sort of factory, and what was a > > couple of functions, some vectors, and an extra constructor turns into > something > > like six classes (SocketFactory, SocketPool, SocketPoolFactory, > > SocketFactoryImpl, SocketPoolImpl, SocketPoolImplFactory). If this complexity > > is okay, I'll go for it, but it seems excessive. > > Could you use lazy allocation? That is, don't pre-allocate sockets, but if the > current pool is less than N, then just allocate a new one. > > I think SocketPool + SocketPoolImpl should suffice. And I'd even be okay if > SocketPoolImpl was hidden in an unnamed namespace inside socket_pool.cc That would make the first N UDP sockets have a predictable pattern, which is the whole reason for having a pool in the first place.
On 2012/09/05 02:50:47, Matt Menke wrote: > On 2012/09/05 02:43:33, szym wrote: > > On 2012/09/05 01:33:15, ttuttle wrote: > > > I just went to do that, and here's the sticky part: If we want to, say, > > > preallocate 256 sockets when we create the buffer/pool/whatever, then it > needs > > a > > > way of allocating sockets, not just putting and getting ones allocated > > > elsewhere. This means we're passing it some sort of factory, and what was a > > > couple of functions, some vectors, and an extra constructor turns into > > something > > > like six classes (SocketFactory, SocketPool, SocketPoolFactory, > > > SocketFactoryImpl, SocketPoolImpl, SocketPoolImplFactory). If this > complexity > > > is okay, I'll go for it, but it seems excessive. > > > > Could you use lazy allocation? That is, don't pre-allocate sockets, but if the > > current pool is less than N, then just allocate a new one. > > > > I think SocketPool + SocketPoolImpl should suffice. And I'd even be okay if > > SocketPoolImpl was hidden in an unnamed namespace inside socket_pool.cc > > That would make the first N UDP sockets have a predictable pattern, which is the > whole reason for having a pool in the first place. OK. Still, why cannot we pass a fully-initialized SocketPool to DnsSession? Why is a SocketPoolFactory needed?
On 2012/09/05 02:57:39, szym wrote: > On 2012/09/05 02:50:47, Matt Menke wrote: > > On 2012/09/05 02:43:33, szym wrote: > > > On 2012/09/05 01:33:15, ttuttle wrote: > > > > I just went to do that, and here's the sticky part: If we want to, say, > > > > preallocate 256 sockets when we create the buffer/pool/whatever, then it > > needs > > > a > > > > way of allocating sockets, not just putting and getting ones allocated > > > > elsewhere. This means we're passing it some sort of factory, and what was > a > > > > couple of functions, some vectors, and an extra constructor turns into > > > something > > > > like six classes (SocketFactory, SocketPool, SocketPoolFactory, > > > > SocketFactoryImpl, SocketPoolImpl, SocketPoolImplFactory). If this > > complexity > > > > is okay, I'll go for it, but it seems excessive. > > > > > > Could you use lazy allocation? That is, don't pre-allocate sockets, but if > the > > > current pool is less than N, then just allocate a new one. > > > > > > I think SocketPool + SocketPoolImpl should suffice. And I'd even be okay if > > > SocketPoolImpl was hidden in an unnamed namespace inside socket_pool.cc > > > > That would make the first N UDP sockets have a predictable pattern, which is > the > > whole reason for having a pool in the first place. > > OK. Still, why cannot we pass a fully-initialized SocketPool to DnsSession? Why > is a SocketPoolFactory needed? First, we'd be passing several pools, not just one, because each pool holds sockets for a single nameserver. This would mean that: 1. The caller and the session both need to know how to allocate and connect sockets. This seems redundant. 2. We are handing DnsSession a massive amount of what I think should be internal state, which it needs to be paranoid about -- the vector of pools could be the wrong size, the sockets could be in the wrong pools, and so on.
Given that DnsSession::AllocateSocket/ReleaseSocket take an |index| argument, I think it'd make things simpler to have SocketPool internally manage all the per-nameserver pools, and DnsSession be a dumb forwarder. Instead of having the explicit construction of each per-server pool in DnsSession, you could just call SocketPool::Initialize(size_t num_nameservers). This would make it easy to test SocketPoolImpl and minimize the boiler plate. If you don't want to make a net/dns/socket_pool* you can keep it as DnsSession::SocketPool and DnsSession::SocketPoolImpl. For dns_transaction_test I suggest creating MockSocketPool implementation. http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.cc File net/dns/dns_session.cc (right): http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.cc#new... net/dns/dns_session.cc:112: /* NB: This callback is also used for selecting sockets from the pool. */ I think the RNG used for selecting sockets from the pool should be internal to SocketPool. If you intend to test if the order of selection matches the outputs of the RNG, it would probably be best if you made SocketPool a proper class. It can be an inner class (since it won't be used outside of DnsSession), but it would be easier to test. nit: code style forbids /* this style of comments */ http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.cc#new... net/dns/dns_session.cc:208: CHECK(index >= 0); Consider making |index| unsigned. http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.cc#new... net/dns/dns_session.cc:214: if (socket.get()) nit: needs braces. http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.h File net/dns/dns_session.h (right): http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.h#newc... net/dns/dns_session.h:34: //typedef base::Callback<SocketFactoryFunction> SocketFactoryCallback; What are these comments for? http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.h#newc... net/dns/dns_session.h:36: typedef DatagramClientSocket* EndPointSocketFactoryFunction(void); Do you really need this typedef? Is it used only in the one typedef below? http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.h#newc... net/dns/dns_session.h:42: typedef base::Callback<SocketPoolFactoryFunction> SocketPoolFactoryCallback; A factory function is an unusual pattern. I think you should stick to ABC. This also needs some comment explaining why SocketPoolFactoryFunction takes EndPointSocketFactoryCallback. http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.h#newc... net/dns/dns_session.h:62: virtual void PutSocket(scoped_ptr<DatagramClientSocket> socket) = 0; I think AllocateSocket/ReleaseSocket would be more clear. http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.h#newc... net/dns/dns_session.h:65: // Use the built-in socket pool class with the default size nit: End sentences with periods. http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session.h#newc... net/dns/dns_session.h:76: NetLog* net_log); What do you need this constructor for? If only for dns_transaction_unittest.cc then I suggest creating a MockSocketPoolFactoryCallback. http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session_unitte... File net/dns/dns_session_unittest.cc (right): http://codereview.chromium.org/10878090/diff/13002/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:120: socket_factory_->ExpectSocketsCreated(num); Wrapping EXPECT_* in methods makes it more difficult to track down where the test fails, because the line printed will be from here, not from where the Expect* wrapper is called. If possible, have it return a boolean and EXPECT_* it in the test body.
https://chromiumcodereview.appspot.com/10878090/diff/13002/net/dns/dns_sessio... File net/dns/dns_session.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/13002/net/dns/dns_sessio... net/dns/dns_session.cc:208: CHECK(index >= 0); On 2012/09/10 19:31:00, szym wrote: > Consider making |index| unsigned. Done. https://chromiumcodereview.appspot.com/10878090/diff/13002/net/dns/dns_sessio... File net/dns/dns_session_unittest.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/13002/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:120: socket_factory_->ExpectSocketsCreated(num); On 2012/09/10 19:31:00, szym wrote: > Wrapping EXPECT_* in methods makes it more difficult to track down where the > test fails, because the line printed will be from here, not from where the > Expect* wrapper is called. If possible, have it return a boolean and EXPECT_* it > in the test body. Done.
http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session.h File net/dns/dns_session.h (right): http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session.h#newc... net/dns/dns_session.h:30: class SocketPool; leftovers? http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session.h#newc... net/dns/dns_session.h:46: scoped_ptr<DatagramClientSocket> socket_; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session.h#newc... net/dns/dns_session.h:50: ClientSocketFactory* socket_factory, DnsSession never uses ClientSocketFactory other than to Initialize the DnsSocketPool. I suggest you pass ClientSocketFactory to DnsSocketPool constructor. Could consider doing the same with |config.nameservers|, but in that case passing it from DnsSession ensures consistency. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session.h#newc... net/dns/dns_session.h:80: RandIntCallback rand_int_callback_; Do you still need this change? http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... File net/dns/dns_session_unittest.cc (right): http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:20: class TestClientSocketFactory : public MockClientSocketFactory { Why are you deriving from MockClientSocketFactory? If you aren't really using it, derive from ClientSocketFactory and add NOTIMPLEMENTED() to the calls you don't want to implement. Alternatively use MockClientSocketFactory as is (just beware that it does not own SocketDataProviders either!) http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:22: virtual DatagramClientSocket* CreateDatagramClientSocket( You will need to add a virtual destructor for this to compile on clang. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:30: enum { ALLOCATE, FREE } action_; nit: public fields should have no "_" suffix. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:40: void Initialize(unsigned char num_servers); unsigned char? make it unsigned (int). If you want to ensure it's small add a comment and a DCHECK in implementation. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:42: bool Allocated(unsigned server_index); I'm ok with Allocated, but perhaps "DidAllocate" is more explicit. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:52: bool ExpectEvent(const PoolEvent& event); suggest: "ReceivedEvent" or "EventHappened" etc. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:58: NullDnsSocketPool(DnsSessionTest* test) : test_(test) { } need virtual destructor http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:81: for (unsigned char i = 0; i < num_servers; i++) { nit: ++i http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:92: static_cast<DnsSocketPool*>(new NullDnsSocketPool(this)); No need for static_cast to base class. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:118: bool DnsSessionTest::Ready() { suggest: "NoMoreEvents" http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:154: SocketDataProvider* data_provider = new StaticSocketDataProvider(); You are leaking |data_provider|. MockUDPClientSocket won't own it. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:164: * ...while in DnsSession or separately? Since you moved DnsSocketPool implementation to dns_socket_pool.cc, I suggest testing it in dns_socket_pool_unittest.cc http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:190: } nit: // namespace http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:192: } nit: // namespace net http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc File net/dns/dns_socket_pool.cc (right): http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:29: CHECK(socket_factory && nameservers); use separate CHECKs, should probably use DCHECKs instead. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:43: static const DatagramSocket::BindType kBindType = DatagramSocket::DEFAULT_BIND; suggest anonymous namespace at the top of this file instead of static http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:56: CHECK(server_index < nameservers_->size()); Should probably use DCHECK. Use DCHECK_LT http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:58: DatagramClientSocket* socket; Use scoped_ptr here so that you don't need to worry about wrapping it or cleaning it up. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:66: return scoped_ptr<DatagramClientSocket>(NULL); nit: indent http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:84: const std::vector<IPEndPoint>* nameservers); All three need OVERRIDE. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:90: private: Add empty line above. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:91: std::vector<std::vector<DatagramClientSocket*> > pools_; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:91: std::vector<std::vector<DatagramClientSocket*> > pools_; All of the sockets leak out in ~DefaultDnsSocketPool. I wonder if it wouldn't be better to use scoped_array<ScopedVector<DatagramClientSocket> > You are already doing bound checking yourself. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:97: DnsSocketPool* pool = static_cast<DnsSocketPool*>(default_pool); no need for static_cast to base class pointer http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:98: return scoped_ptr<DnsSocketPool>(pool); scoped_ptr<DnsSocketPool>(new DefaultDnsSocketPool()) should just work http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:104: const std::vector<IPEndPoint>* nameservers) OVERRIDE { I suggest you DCHECK that |pools_| is empty to enforce you can Initialize only once. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:114: if (!socket) { nit: no need for {} http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:124: CHECK(server_index < pools_.size()); DCHECK_LT http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:127: // allocate one extra socket, since we're about to remove one You say "one extra" but this is a loop. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:131: if (!socket) { nit: no need for {} http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:138: LOG(WARNING) << "Low DNS port entropy: " << "wanted " << kAllocateMinSize nit: no need for the second << http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:153: CHECK(server_index < pools_.size()); DCHECK_LT http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:156: if (pool.size() < kRetainMaxSize) { nit: no need for {} http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.h File net/dns/dns_socket_pool.h (right): http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.h#... net/dns/dns_socket_pool.h:21: static scoped_ptr<DnsSocketPool> MakeDefault(); suggest: CreateDefault to avoid ambiguity http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.h#... net/dns/dns_socket_pool.h:26: const std::vector<IPEndPoint>* nameservers); I suggest taking a reference and making a copy. Although not memory efficient it requires less reasoning about lifetime. I'd also avoid have some implementation in the base class to be overridden by derived classes. Make this protected: InitializeInternal and expose pure virtual Initialize. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.h#... net/dns/dns_socket_pool.h:45: bool initialized_; DISALLOW_COPY_AND_ASSIGN
https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_session.h File net/dns/dns_session.h (right): https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session.h:30: class SocketPool; On 2012/09/14 20:49:27, szym wrote: > leftovers? Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session.h:46: scoped_ptr<DatagramClientSocket> socket_; On 2012/09/14 20:49:27, szym wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session.h:50: ClientSocketFactory* socket_factory, On 2012/09/14 20:49:27, szym wrote: > DnsSession never uses ClientSocketFactory other than to Initialize the > DnsSocketPool. I suggest you pass ClientSocketFactory to DnsSocketPool > constructor. Could consider doing the same with |config.nameservers|, but in > that case passing it from DnsSession ensures consistency. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session.h:80: RandIntCallback rand_int_callback_; On 2012/09/14 20:49:27, szym wrote: > Do you still need this change? Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... File net/dns/dns_session_unittest.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:20: class TestClientSocketFactory : public MockClientSocketFactory { On 2012/09/14 20:49:27, szym wrote: > Why are you deriving from MockClientSocketFactory? If you aren't really using > it, derive from ClientSocketFactory and add NOTIMPLEMENTED() to the calls you > don't want to implement. > > Alternatively use MockClientSocketFactory as is (just beware that it does not > own SocketDataProviders either!) Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:22: virtual DatagramClientSocket* CreateDatagramClientSocket( On 2012/09/14 20:49:27, szym wrote: > You will need to add a virtual destructor for this to compile on clang. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:30: enum { ALLOCATE, FREE } action_; On 2012/09/14 20:49:27, szym wrote: > nit: public fields should have no "_" suffix. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:40: void Initialize(unsigned char num_servers); On 2012/09/14 20:49:27, szym wrote: > unsigned char? make it unsigned (int). If you want to ensure it's small add a > comment and a DCHECK in implementation. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:42: bool Allocated(unsigned server_index); On 2012/09/14 20:49:27, szym wrote: > I'm ok with Allocated, but perhaps "DidAllocate" is more explicit. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:52: bool ExpectEvent(const PoolEvent& event); On 2012/09/14 20:49:27, szym wrote: > suggest: "ReceivedEvent" or "EventHappened" etc. The problem with those is that either could be used for "we just got the event" in addition to "we're checking if that event happened". What about something like PopEvent? https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:58: NullDnsSocketPool(DnsSessionTest* test) : test_(test) { } On 2012/09/14 20:49:27, szym wrote: > need virtual destructor Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:81: for (unsigned char i = 0; i < num_servers; i++) { On 2012/09/14 20:49:27, szym wrote: > nit: ++i Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:92: static_cast<DnsSocketPool*>(new NullDnsSocketPool(this)); On 2012/09/14 20:49:27, szym wrote: > No need for static_cast to base class. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:118: bool DnsSessionTest::Ready() { On 2012/09/14 20:49:27, szym wrote: > suggest: "NoMoreEvents" Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:154: SocketDataProvider* data_provider = new StaticSocketDataProvider(); On 2012/09/14 20:49:27, szym wrote: > You are leaking |data_provider|. MockUDPClientSocket won't own it. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:190: } On 2012/09/14 20:49:27, szym wrote: > nit: // namespace Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:192: } On 2012/09/14 20:49:27, szym wrote: > nit: // namespace net Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... File net/dns/dns_socket_pool.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:29: CHECK(socket_factory && nameservers); On 2012/09/14 20:49:27, szym wrote: > use separate CHECKs, should probably use DCHECKs instead. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:43: static const DatagramSocket::BindType kBindType = DatagramSocket::DEFAULT_BIND; On 2012/09/14 20:49:27, szym wrote: > suggest anonymous namespace at the top of this file instead of static Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:56: CHECK(server_index < nameservers_->size()); On 2012/09/14 20:49:27, szym wrote: > Should probably use DCHECK. Use DCHECK_LT Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:58: DatagramClientSocket* socket; On 2012/09/14 20:49:27, szym wrote: > Use scoped_ptr here so that you don't need to worry about wrapping it or > cleaning it up. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:66: return scoped_ptr<DatagramClientSocket>(NULL); On 2012/09/14 20:49:27, szym wrote: > nit: indent Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:84: const std::vector<IPEndPoint>* nameservers); On 2012/09/14 20:49:27, szym wrote: > All three need OVERRIDE. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:90: private: On 2012/09/14 20:49:27, szym wrote: > Add empty line above. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:91: std::vector<std::vector<DatagramClientSocket*> > pools_; On 2012/09/14 20:49:27, szym wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:97: DnsSocketPool* pool = static_cast<DnsSocketPool*>(default_pool); On 2012/09/14 20:49:27, szym wrote: > no need for static_cast to base class pointer Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:98: return scoped_ptr<DnsSocketPool>(pool); On 2012/09/14 20:49:27, szym wrote: > scoped_ptr<DnsSocketPool>(new DefaultDnsSocketPool()) should just work Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:104: const std::vector<IPEndPoint>* nameservers) OVERRIDE { On 2012/09/14 20:49:27, szym wrote: > I suggest you DCHECK that |pools_| is empty to enforce you can Initialize only > once. InitializeInternal already keeps a flag and enforces that we can only initialize once. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:114: if (!socket) { On 2012/09/14 20:49:27, szym wrote: > nit: no need for {} Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:124: CHECK(server_index < pools_.size()); On 2012/09/14 20:49:27, szym wrote: > DCHECK_LT Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:127: // allocate one extra socket, since we're about to remove one On 2012/09/14 20:49:27, szym wrote: > You say "one extra" but this is a loop. The comment is to explain "kAllocateMinSize + 1". https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:138: LOG(WARNING) << "Low DNS port entropy: " << "wanted " << kAllocateMinSize On 2012/09/14 20:49:27, szym wrote: > nit: no need for the second << Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:153: CHECK(server_index < pools_.size()); On 2012/09/14 20:49:27, szym wrote: > DCHECK_LT Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:156: if (pool.size() < kRetainMaxSize) { On 2012/09/14 20:49:27, szym wrote: > nit: no need for {} Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... File net/dns/dns_socket_pool.h (right): https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.h:21: static scoped_ptr<DnsSocketPool> MakeDefault(); On 2012/09/14 20:49:27, szym wrote: > suggest: CreateDefault to avoid ambiguity Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.h:26: const std::vector<IPEndPoint>* nameservers); On 2012/09/14 20:49:27, szym wrote: > I suggest taking a reference and making a copy. Although not memory efficient it > requires less reasoning about lifetime. > > I'd also avoid have some implementation in the base class to be overridden by > derived classes. Make this protected: InitializeInternal and expose pure virtual > Initialize. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.h:45: bool initialized_; On 2012/09/14 20:49:27, szym wrote: > DISALLOW_COPY_AND_ASSIGN Done.
PTAL.
Sorry for the delay. Note the IMPORTANT leak. http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc File net/dns/dns_socket_pool.cc (right): http://codereview.chromium.org/10878090/diff/21001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:91: std::vector<std::vector<DatagramClientSocket*> > pools_; On 2012/09/14 20:49:27, szym wrote: > All of the sockets leak out in ~DefaultDnsSocketPool. > > I wonder if it wouldn't be better to use > scoped_array<ScopedVector<DatagramClientSocket> > > You are already doing bound checking yourself. Missed the one about leak. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_client.cc File net/dns/dns_client.cc (right): http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_client.cc#newc... net/dns/dns_client.cc:33: DnsSocketPool::CreateDefault(factory).Pass(), You don't need Pass() here. It's an Rvalue. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session.cc File net/dns/dns_session.cc (right): http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session.cc#new... net/dns/dns_session.cc:63: socket = socket_pool_->AllocateSocket(server_index).Pass(); You don't need Pass() here. It's an Rvalue. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session.cc#new... net/dns/dns_session.cc:64: if (!socket.get()) { nit: no need for {} http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session.cc#new... net/dns/dns_session.cc:80: CHECK(socket.get()); I think DCHECK would suffice. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session.h File net/dns/dns_session.h (right): http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session.h#newc... net/dns/dns_session.h:66: // Allocate a socket, already connected to the server address. Add remark here or above class SocketLease explaining that the socket is released when the SocketLease is destroyed. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session_unitte... File net/dns/dns_session_unittest.cc (right): http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:35: NetLog*, const NetLog::Source&) { nit: OVERRIDE http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:44: const SSLClientSocketContext& context) { nit: OVERRIDE http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:49: virtual void ClearSSLSessionCache() { NOTIMPLEMENTED(); } nit: OVERRIDE http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:82: class NullDnsSocketPool : public DnsSocketPool { I suggest Mock rather than Null. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:98: return CreateConnectedSocket(server_index).Pass(); No need for Pass(). http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_session_unitte... net/dns/dns_session_unittest.cc:218: EXPECT_TRUE(NoMoreEvents()); Since all that's tested here is whether DnsSession and SocketLease transparently execute Allocate/Free calls on the DnsSocketPool, I wonder if using gmock wouldn't be more suitable. Having never used gmock myself, I'm leaving this up to you. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.cc File net/dns/dns_socket_pool.cc (right): http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:24: const unsigned kInitialPoolSize = 256; Some comment explaining how these constants will be used would help. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:46: // net_log can be NULL if we don't want to log If you want to keep this comment put it in the declaration in the header. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:49: CHECK(!initialized_); A DCHECK would probably suffice for now. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:84: IMPORTANT: all the retained sockets are leaking out on destruction. Add a destructor. Make it virtual. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:97: std::vector<std::vector<DatagramClientSocket*> > pools_; vector<DatagramClientSocket*> is used 4 times. I suggest a typedef SocketVector. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:134: // Allocate one socket more than the minimum, since we're about to remove one I'm guessing this is so that if kAllocateMinSize == 0, you still allocate one socket to be used. If that's the reasoning, I suggest defining kAllocateMinSize as the "minimum number of sockets to allocate from", and setting it to 1 on !OS_WIN. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:138: if (!socket) { nit: no need for {} (at least for consistency with above) http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:144: if (pool.size() < kAllocateMinSize) { No +1 here, so a bit confused. If you removed +1, you wouldn't need extra comments. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.h File net/dns/dns_socket_pool.h (right): http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.h#... net/dns/dns_socket_pool.h:23: IMPORTANT: Needs a virtual destructor. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.h#... net/dns/dns_socket_pool.h:25: NetLog* net_log, I suggest reversing the order of arguments here. |net_log| is essentially optional (can be set to NULL). http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_socket_pool.h#... net/dns/dns_socket_pool.h:56: #endif nit: add // NET_DNS_DNS... http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_transaction.cc File net/dns/dns_transaction.cc (right): http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_transaction.cc... net/dns/dns_transaction.cc:89: const DatagramClientSocket* socket() const { This is used only to get the NetLog::Source. While you are here, maybe make it directly return a NetLog::Source? Then you could use (non-const) socket() instead of my_socket() for the internal calls. http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_transaction_un... File net/dns/dns_transaction_unittest.cc (right): http://codereview.chromium.org/10878090/diff/24001/net/dns/dns_transaction_un... net/dns/dns_transaction_unittest.cc:303: DnsSocketPool::CreateDefault(socket_factory_.get()).Pass(), No need for Pass().
PTAL. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... File net/dns/dns_socket_pool.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:91: std::vector<std::vector<DatagramClientSocket*> > pools_; On 2012/09/26 11:16:12, szym wrote: > On 2012/09/14 20:49:27, szym wrote: > > All of the sockets leak out in ~DefaultDnsSocketPool. > > > > I wonder if it wouldn't be better to use > > scoped_array<ScopedVector<DatagramClientSocket> > > > You are already doing bound checking yourself. > > Missed the one about leak. Done. https://chromiumcodereview.appspot.com/10878090/diff/21001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:131: if (!socket) { On 2012/09/14 20:49:27, szym wrote: > nit: no need for {} Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_client.cc File net/dns/dns_client.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_client... net/dns/dns_client.cc:33: DnsSocketPool::CreateDefault(factory).Pass(), On 2012/09/26 11:16:12, szym wrote: > You don't need Pass() here. It's an Rvalue. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... File net/dns/dns_session.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session.cc:63: socket = socket_pool_->AllocateSocket(server_index).Pass(); On 2012/09/26 11:16:12, szym wrote: > You don't need Pass() here. It's an Rvalue. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session.cc:64: if (!socket.get()) { On 2012/09/26 11:16:12, szym wrote: > nit: no need for {} Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session.cc:80: CHECK(socket.get()); On 2012/09/26 11:16:12, szym wrote: > I think DCHECK would suffice. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_session.h File net/dns/dns_session.h (right): https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session.h:66: // Allocate a socket, already connected to the server address. On 2012/09/26 11:16:12, szym wrote: > Add remark here or above class SocketLease explaining that the socket is > released when the SocketLease is destroyed. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... File net/dns/dns_session_unittest.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:35: NetLog*, const NetLog::Source&) { On 2012/09/26 11:16:12, szym wrote: > nit: OVERRIDE Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:44: const SSLClientSocketContext& context) { On 2012/09/26 11:16:12, szym wrote: > nit: OVERRIDE Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:49: virtual void ClearSSLSessionCache() { NOTIMPLEMENTED(); } On 2012/09/26 11:16:12, szym wrote: > nit: OVERRIDE Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:82: class NullDnsSocketPool : public DnsSocketPool { On 2012/09/26 11:16:12, szym wrote: > I suggest Mock rather than Null. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:98: return CreateConnectedSocket(server_index).Pass(); On 2012/09/26 11:16:12, szym wrote: > No need for Pass(). Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_sessio... net/dns/dns_session_unittest.cc:218: EXPECT_TRUE(NoMoreEvents()); On 2012/09/26 11:16:12, szym wrote: > Since all that's tested here is whether DnsSession and SocketLease transparently > execute Allocate/Free calls on the DnsSocketPool, I wonder if using gmock > wouldn't be more suitable. Having never used gmock myself, I'm leaving this up > to you. I'm going to leave this, since it works and is not unreasonable. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... File net/dns/dns_socket_pool.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:24: const unsigned kInitialPoolSize = 256; On 2012/09/26 11:16:12, szym wrote: > Some comment explaining how these constants will be used would help. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:46: // net_log can be NULL if we don't want to log On 2012/09/26 11:16:12, szym wrote: > If you want to keep this comment put it in the declaration in the header. Deleting it instead. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:49: CHECK(!initialized_); On 2012/09/26 11:16:12, szym wrote: > A DCHECK would probably suffice for now. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:84: On 2012/09/26 11:16:12, szym wrote: > IMPORTANT: all the retained sockets are leaking out on destruction. Add a > destructor. Make it virtual. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:97: std::vector<std::vector<DatagramClientSocket*> > pools_; On 2012/09/26 11:16:12, szym wrote: > vector<DatagramClientSocket*> is used 4 times. I suggest a typedef SocketVector. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:134: // Allocate one socket more than the minimum, since we're about to remove one On 2012/09/26 11:16:12, szym wrote: > I'm guessing this is so that if kAllocateMinSize == 0, you still allocate one > socket to be used. If that's the reasoning, I suggest defining kAllocateMinSize > as the "minimum number of sockets to allocate from", and setting it to 1 on > !OS_WIN. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:138: if (!socket) { On 2012/09/26 11:16:12, szym wrote: > nit: no need for {} (at least for consistency with above) Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.cc:144: if (pool.size() < kAllocateMinSize) { On 2012/09/26 11:16:12, szym wrote: > No +1 here, so a bit confused. If you removed +1, you wouldn't need extra > comments. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... File net/dns/dns_socket_pool.h (right): https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.h:23: On 2012/09/26 11:16:12, szym wrote: > IMPORTANT: Needs a virtual destructor. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.h:25: NetLog* net_log, On 2012/09/26 11:16:12, szym wrote: > I suggest reversing the order of arguments here. |net_log| is essentially > optional (can be set to NULL). Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_socket... net/dns/dns_socket_pool.h:56: #endif On 2012/09/26 11:16:12, szym wrote: > nit: add // NET_DNS_DNS... Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_transa... File net/dns/dns_transaction.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_transa... net/dns/dns_transaction.cc:89: const DatagramClientSocket* socket() const { On 2012/09/26 11:16:12, szym wrote: > This is used only to get the NetLog::Source. While you are here, maybe make it > directly return a NetLog::Source? Then you could use (non-const) socket() > instead of my_socket() for the internal calls. Done. https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_transa... File net/dns/dns_transaction_unittest.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/24001/net/dns/dns_transa... net/dns/dns_transaction_unittest.cc:303: DnsSocketPool::CreateDefault(socket_factory_.get()).Pass(), On 2012/09/26 11:16:12, szym wrote: > No need for Pass(). Done.
Please fix the destructor and lint errors. Otherwise LGTM. https://chromiumcodereview.appspot.com/10878090/diff/36002/net/dns/dns_socket... File net/dns/dns_socket_pool.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/36002/net/dns/dns_socket... net/dns/dns_socket_pool.cc:40: typedef std::vector<DatagramClientSocket*> SocketVector; When reading, I'd expect this typedef to be inside of DefaultDnsSocketPool, right before it's instantiated. https://chromiumcodereview.appspot.com/10878090/diff/36002/net/dns/dns_socket... File net/dns/dns_socket_pool.h (right): https://chromiumcodereview.appspot.com/10878090/diff/36002/net/dns/dns_socket... net/dns/dns_socket_pool.h:21: ~DnsSocketPool() { } IMPORTANT: This has to be virtual to work.
Please hold off committing this until http://crbug.com/151896 is fixed.
PTAL. https://chromiumcodereview.appspot.com/10878090/diff/36002/net/dns/dns_socket... File net/dns/dns_socket_pool.cc (right): https://chromiumcodereview.appspot.com/10878090/diff/36002/net/dns/dns_socket... net/dns/dns_socket_pool.cc:40: typedef std::vector<DatagramClientSocket*> SocketVector; On 2012/09/26 21:03:33, szym wrote: > When reading, I'd expect this typedef to be inside of DefaultDnsSocketPool, > right before it's instantiated. Done. https://chromiumcodereview.appspot.com/10878090/diff/36002/net/dns/dns_socket... File net/dns/dns_socket_pool.h (right): https://chromiumcodereview.appspot.com/10878090/diff/36002/net/dns/dns_socket... net/dns/dns_socket_pool.h:21: ~DnsSocketPool() { } On 2012/09/26 21:03:33, szym wrote: > IMPORTANT: This has to be virtual to work. Done.
http://codereview.chromium.org/10878090/diff/41001/net/dns/dns_socket_pool.cc File net/dns/dns_socket_pool.cc (right): http://codereview.chromium.org/10878090/diff/41001/net/dns/dns_socket_pool.cc... net/dns/dns_socket_pool.cc:152: NetLog* net_log) OVERRIDE { OVERRIDE is only used/allowed on declaration not definition. You should probably run "git cl try" at this point to catch those issues.
PTAL.
http://codereview.chromium.org/10878090/diff/57001/net/dns/dns_socket_pool.h File net/dns/dns_socket_pool.h (right): http://codereview.chromium.org/10878090/diff/57001/net/dns/dns_socket_pool.h#... net/dns/dns_socket_pool.h:20: class NET_EXPORT_PRIVATE DnsSocketPool { Please provide a brief comment about what this class is used for, as well as the public methods.
I think it's a good time to land this now, when there are no pending CLs related to DnsTransaction. Unless there are further requests here, please add comments as per cbentzel's request, fix the CL commit message, and land when ready :)
PTAL. (Added comments.)
On 2012/11/07 20:25:11, ttuttle wrote: > PTAL. (Added comments.) Comments look good, thanks.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/10878090/69001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
My clone of this CL: https://codereview.chromium.org/11573012/ seems to be compiling on all trybots. Let's give it another go.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/10878090/90001
Sorry for I got bad news for ya. Compile failed with a clobber build on win. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
https://codereview.chromium.org/10878090/diff/90001/net/dns/dns_session.h File net/dns/dns_session.h (right): https://codereview.chromium.org/10878090/diff/90001/net/dns/dns_session.h#new... net/dns/dns_session.h:32: class SocketLease { NET_EXPORT_PRIVATE got lost in the rebase?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ttuttle@chromium.org/10878090/91004
Message was sent while issue was closed.
Change committed as 173038 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
