Create WebSocketTransportClientSocketPool

Create WebSocketTransportClientSocketPool

It is a variant of TransportClientSocketPool that performs RFC6455-style
connection throttling.

Design doc: https://docs.google.com/a/chromium.org/document/d/1a8sUFQsbN5uve7ziW61ATkrFr3o9A-Tiyw8ig6T3puA/edit

BUG=343107
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279524

[tyoshino (SeeGerritForStatus), 2014-05-22 05:20:03]

https://codereview.chromium.org/240873003/diff/100001/net/socket/client_socke...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/240873003/diff/100001/net/socket/client_socke...
net/socket/client_socket_pool_manager_impl.cc:92: net_log)),
please revert style only change so that reviewers won't be distracted for them.

feel free to send style only fixes if you found some and weigh on your mind.

[tyoshino (SeeGerritForStatus), 2014-05-22 06:04:00]

https://codereview.chromium.org/240873003/diff/100001/base/containers/linked_...
File base/containers/linked_list.h (right):

https://codereview.chromium.org/240873003/diff/100001/base/containers/linked_...
base/containers/linked_list.h:112: this->previous_ = NULL;
use 0 or NULL for both initializer list and here?

btw, what is this change for? debugging?

https://codereview.chromium.org/240873003/diff/100001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/100001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:502:
DCHECK(!connect_timing_.connect_start.is_null());
please try to reduce code duplication even by splitting out some from
TransportClientSocketPool.

it might make the change ugly but also would make it clear which part of your
change is essence and help us figure out the right design, maybe.

[Adam Rice, 2014-05-22 06:49:07]

https://codereview.chromium.org/240873003/diff/100001/base/containers/linked_...
File base/containers/linked_list.h (right):

https://codereview.chromium.org/240873003/diff/100001/base/containers/linked_...
base/containers/linked_list.h:112: this->previous_ = NULL;
On 2014/05/22 06:04:01, tyoshino wrote:
> use 0 or NULL for both initializer list and here?
> 
> btw, what is this change for? debugging?

Okay, I changed the initialisers to use NULL for style-guide compliance.

This change provides the invariant that next() and previous() are non-NULL if
and only if the node is not in a list, which my code depends on. I have added a
comment and a test.

https://codereview.chromium.org/240873003/diff/100001/net/socket/client_socke...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/240873003/diff/100001/net/socket/client_socke...
net/socket/client_socket_pool_manager_impl.cc:92: net_log)),
On 2014/05/22 05:20:03, tyoshino wrote:
> please revert style only change so that reviewers won't be distracted for
them.
> 
> feel free to send style only fixes if you found some and weigh on your mind.

Done.

[Adam Rice, 2014-05-22 14:08:00]

https://codereview.chromium.org/240873003/diff/100001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/100001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:502:
DCHECK(!connect_timing_.connect_start.is_null());
On 2014/05/22 06:04:01, tyoshino wrote:
> please try to reduce code duplication even by splitting out some from
> TransportClientSocketPool.
> 
> it might make the change ugly but also would make it clear which part of your
> change is essence and help us figure out the right design, maybe.

Okay, I have split out the common code from here and other places.

[Adam Rice, 2014-06-10 07:04:30]

Ready for review. PTAL.

[tyoshino (SeeGerritForStatus), 2014-06-11 03:32:54]

https://codereview.chromium.org/240873003/diff/220001/net/socket/client_socke...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/client_socke...
net/socket/client_socket_pool_manager_impl.cc:76: net_log)),
do we need to set WebSocketTransportClientSocketPool to transport_socket_pool_?

[Adam Rice, 2014-06-11 05:33:42]

https://codereview.chromium.org/240873003/diff/220001/net/socket/client_socke...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/client_socke...
net/socket/client_socket_pool_manager_impl.cc:76: net_log)),
On 2014/06/11 03:32:54, tyoshino wrote:
> do we need to set WebSocketTransportClientSocketPool to
transport_socket_pool_?

It is used in client_socket_pool_manager.cc to implement InitSocketPoolHelper().
It may be possible to export it separately, but it would involve even more
extensive changes.

[tyoshino (SeeGerritForStatus), 2014-06-11 06:46:45]

https://codereview.chromium.org/240873003/diff/220001/net/socket/client_socke...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/client_socke...
net/socket/client_socket_pool_manager_impl.cc:63: ?
static_cast<TransportClientSocketPool*>(
remove static_cast?

https://codereview.chromium.org/240873003/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:60: class NET_EXPORT_PRIVATE
TransportConnectJobCommon {
What do you think if we follow the same naming convention as
ClientSocketPoolBase? I.e. TransportConnectJobBase and base_ to hold it. I don't
have strong opinion. If you think Common would be better, I'm ok with it.

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:120: typedef
TransportSocketParams SocketParams;
remove?

[tyoshino (SeeGerritForStatus), 2014-06-11 06:55:05]

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:680: void
WebSocketTransportClientSocketPool::AddHigherLayeredPool(
no need to override?

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:687: void
WebSocketTransportClientSocketPool::RemoveHigherLayeredPool(
no need to override?

[tyoshino (SeeGerritForStatus), 2014-06-11 07:55:11]

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:574:
connect_job->net_log().source().ToEventParametersCallback());
should we log this even when rv == ERR_IO_PENDING?

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:796: void
WebSocketTransportClientSocketPool::CancelJob(ClientSocketHandle* handle) {
merge to CancelRequest?

[Adam Rice, 2014-06-11 08:17:09]

https://codereview.chromium.org/240873003/diff/220001/net/socket/client_socke...
File net/socket/client_socket_pool_manager_impl.cc (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/client_socke...
net/socket/client_socket_pool_manager_impl.cc:63: ?
static_cast<TransportClientSocketPool*>(
On 2014/06/11 06:46:45, tyoshino wrote:
> remove static_cast?

I didn't think it would compile without the cast. I was wrong. Thanks.

https://codereview.chromium.org/240873003/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:60: class NET_EXPORT_PRIVATE
TransportConnectJobCommon {
On 2014/06/11 06:46:45, tyoshino wrote:
> What do you think if we follow the same naming convention as
> ClientSocketPoolBase? I.e. TransportConnectJobBase and base_ to hold it. I
don't
> have strong opinion. If you think Common would be better, I'm ok with it.

I think "Base" strongly implies a base class and so is confusing. So even though
"Base" is used like that in a few places (eg. ClientSocketPoolBase), I would
prefer to use a different word. "Helper" seems to be quite common (eg.
ClientSocketPoolBaseHelper), so I will change it to that unless you have a
better suggestion.

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:574:
connect_job->net_log().source().ToEventParametersCallback());
On 2014/06/11 07:55:11, tyoshino wrote:
> should we log this even when rv == ERR_IO_PENDING?

Because the WebSocketTransportClientSocketPool uses early binding of connections
to handles, I think it is correct to log it here. Regardless of the outcome of
the connect job, it will always be bound to this handle.

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:680: void
WebSocketTransportClientSocketPool::AddHigherLayeredPool(
On 2014/06/11 06:55:04, tyoshino wrote:
> no need to override?

It seemed a little risky to me. The base class implementation could change to
have side-effects. But I think the unit tests would catch any problems. Removed.

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:687: void
WebSocketTransportClientSocketPool::RemoveHigherLayeredPool(
On 2014/06/11 06:55:04, tyoshino wrote:
> no need to override?

Removed.

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:796: void
WebSocketTransportClientSocketPool::CancelJob(ClientSocketHandle* handle) {
On 2014/06/11 07:55:11, tyoshino wrote:
> merge to CancelRequest?

Done.

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:120: typedef
TransportSocketParams SocketParams;
On 2014/06/11 06:46:45, tyoshino wrote:
> remove?

Done.

[tyoshino (SeeGerritForStatus), 2014-06-11 08:22:22]

https://codereview.chromium.org/240873003/diff/220001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:60: class NET_EXPORT_PRIVATE
TransportConnectJobCommon {
On 2014/06/11 08:17:09, Adam Rice wrote:
> On 2014/06/11 06:46:45, tyoshino wrote:
> > What do you think if we follow the same naming convention as
> > ClientSocketPoolBase? I.e. TransportConnectJobBase and base_ to hold it. I
> don't
> > have strong opinion. If you think Common would be better, I'm ok with it.
> 
> I think "Base" strongly implies a base class and so is confusing. So even
though
> "Base" is used like that in a few places (eg. ClientSocketPoolBase), I would
> prefer to use a different word. "Helper" seems to be quite common (eg.
> ClientSocketPoolBaseHelper), so I will change it to that unless you have a
> better suggestion.

fair enough

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/220001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:680: void
WebSocketTransportClientSocketPool::AddHigherLayeredPool(
On 2014/06/11 08:17:09, Adam Rice wrote:
> On 2014/06/11 06:55:04, tyoshino wrote:
> > no need to override?
> 
> It seemed a little risky to me. The base class implementation could change to

Yeah, right, like IsStalled.

> have side-effects. But I think the unit tests would catch any problems.
Removed.

Makes sense.

https://codereview.chromium.org/240873003/diff/240001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/240001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
2014

[Adam Rice, 2014-06-11 08:28:28]

https://codereview.chromium.org/240873003/diff/240001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/240001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:1: // Copyright
(c) 2012 The Chromium Authors. All rights reserved.
On 2014/06/11 08:22:22, tyoshino wrote:
> 2014

Done.

[tyoshino (SeeGerritForStatus), 2014-06-12 06:39:21]

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:54: // the call to
RememberSocket().
maybe this should be

between the call to RememberSocket() and the call to UnlockSocket()

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:124: void GotEndpointLock()
{ OnIOComplete(OK); }
how about DCHECK on next_state_?

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:199:
DCHECK_LT(current_address_index_, static_cast<int>(addresses_.size()));
factor out this DCHECK and addresses_[]?

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:211: int
DoTransportConnect() {
add TODO to update g_last_connect_time and report ConnectInterval

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:249: int
current_address_index_;
how about grouping addresses_ and current_address_index_?

[Adam Rice, 2014-06-12 10:17:33]

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:54: // the call to
RememberSocket().
On 2014/06/12 06:39:20, tyoshino wrote:
> maybe this should be
> 
> between the call to RememberSocket() and the call to UnlockSocket()

Thanks. Done.

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:124: void GotEndpointLock()
{ OnIOComplete(OK); }
On 2014/06/12 06:39:20, tyoshino wrote:
> how about DCHECK on next_state_?

Done.

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:199:
DCHECK_LT(current_address_index_, static_cast<int>(addresses_.size()));
On 2014/06/12 06:39:20, tyoshino wrote:
> factor out this DCHECK and addresses_[]?

Done.

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:211: int
DoTransportConnect() {
On 2014/06/12 06:39:20, tyoshino wrote:
> add TODO to update g_last_connect_time and report ConnectInterval

Done.

https://codereview.chromium.org/240873003/diff/300001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:249: int
current_address_index_;
On 2014/06/12 06:39:20, tyoshino wrote:
> how about grouping addresses_ and current_address_index_?

Done.

[Adam Rice, 2014-06-16 10:20:42]

+jgraettinger, please take a look.

[Adam Rice, 2014-06-16 10:26:17]

+eroman Please review the LinkedList<T> changes.

[Johnny, 2014-06-16 23:41:11]

Looks good in general, though there's a lot here and I'm not confident my review
is exhaustive. Comments inline

https://codereview.chromium.org/240873003/diff/320001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:177: TransportConnectJobHelper data_;
nit: |data_| => |helper_|?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:41: class
WebSocketEndpointLockManager {
Should this be in it's own .h & .cc?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:60: // for the same
|socket|; all calls after the first will be ignored.
Is this expected to happen? If not, why not be more strict?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:100:
CHECK_LE(addresses_.size(),
This is a pretty loose CHECK. I'm not sure what it guards against, in practice.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:253: return result;
Sanity check: Looks like WebSocketBasicHandshakeStream::Upgrade() is responsible
for releasing the lock on success, allowing a new connection to start?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:88: // Called from
fallback_timer_
Nit: Called from |fallback_timer_|.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:96:
TransportConnectJobHelper data_;
Optional: |data_| => |helper_|.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:91: class
MockClientSocket : public StreamSocket {
Do these mocks deviate significantly from transport_client_socket_unittest.cc?
Can they share implementation?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:488: virtual
~WebSocketTransportClientSocketPoolTest() {}
Verify that WebSocketEndpointLockManager is empty/in an expected state?

Also, I don't think I saw tests specifically on WebSocketEndpointLockManager.

[tyoshino (SeeGerritForStatus), 2014-06-18 06:49:43]

https://codereview.chromium.org/240873003/diff/320001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:316: case STATE_RESOLVE_HOST:
DCHECK_EQ(OK, rv);

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:78: EndPointJobMap
endpoint_job_map_;
please explain what an endpoint_job_map_ entry with empty ConnectJobQueue mean.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:112:
WebSocketEndpointLockManager::GetInstance()->UnlockEndpoint(
don't we call UnlockEndpoint() twice when DoTransportConnectComplete() is called
with non-OK result?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:155: STATE_WAIT_FOR_LOCK,
[optional] wondering if s/WAIT/OBTAIN/ sounds better or not

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:186: case
STATE_WAIT_FOR_LOCK:
DCHECK_EQ(OK, rv)

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:189: case
STATE_WAIT_FOR_LOCK_COMPLETE:
DCHECK_EQ(OK, rv)

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:360: //
LOAD_STATE_CONNECTING is better than
could you please elaborate better for what point

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:395: DVLOG(1) <<
"Unexpected ADDRESS_FAMILY: " << it->GetFamily();
do we want to ignore this?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:418: // This use of
base::Unretained is safe because fallback_timer_ is owned
|fallback_timer_|

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:425:
base::Unretained(this)));
return here?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:426: }
how about breaking L417 into two if-s and return when result == ERR_IO_PENDING
&& !ipv4_job_? and then, you can use switch for L409, L417 and L427.

i understand that this code works but i guess it improves readability

it's optional

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:574:
TimeDelta::FromSeconds(kTransportConnectJobTimeoutInSeconds),
ConnectionTimeout()?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:724: if (!handed_out_socket
&& is_stalled_ && !ReachedMaxSocketsLimit())
is this correct? pending_connects_.size() decreases at L727

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:769:
handle->set_idle_time(TimeDelta());
needed?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:17: #include
"base/timer/timer.h"
include net/base/net_log.h as you have BoundNetLog in
WebSocketTransportConnectJob

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:30:
OnHostResolutionCallback;
remove and use TransportClientSocketPool's one in the unittest?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:39: // timer
(kIPv6FallbackTimerInMs) and start a connect() to a IPv4 address if the
an IPv4?

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:100: // go first, followed
after |kIPv6FallbackTimerInMs| by the IPV4
IPv4

https://codereview.chromium.org/240873003/diff/320001/net/websockets/websocke...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/240873003/diff/320001/net/websockets/websocke...
net/websockets/websocket_stream_test.cc:89: ->CloseAllConnections();
please write the reason why you need this

[Adam Rice, 2014-06-19 13:55:21]

Sorry for the volume of changes. There's more files, but less lines in each one.

https://codereview.chromium.org/240873003/diff/320001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:177: TransportConnectJobHelper data_;
On 2014/06/16 23:41:10, Johnny wrote:
> nit: |data_| => |helper_|?

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:316: case STATE_RESOLVE_HOST:
On 2014/06/18 06:49:42, tyoshino wrote:
> DCHECK_EQ(OK, rv);

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:41: class
WebSocketEndpointLockManager {
On 2014/06/16 23:41:11, Johnny wrote:
> Should this be in it's own .h & .cc?

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:60: // for the same
|socket|; all calls after the first will be ignored.
On 2014/06/16 23:41:11, Johnny wrote:
> Is this expected to happen? If not, why not be more strict?

I clarified the comment. In the case of a successful WebSocket connection, this
method is called twice.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:78: EndPointJobMap
endpoint_job_map_;
On 2014/06/18 06:49:42, tyoshino wrote:
> please explain what an endpoint_job_map_ entry with empty ConnectJobQueue
mean.

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:100:
CHECK_LE(addresses_.size(),
On 2014/06/16 23:41:11, Johnny wrote:
> This is a pretty loose CHECK. I'm not sure what it guards against, in
practice.

current_address_index_ was an int in imitation of TCPClientSocket. I don't think
it's possible for the resolver to return 2 billion addresses for a host, but if
it did, bad things would happen.

Unlike TCPClientSocket, this class never requires current_address_index_ to be
negative, so I changed it to a size_t to make the code cleaner.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:112:
WebSocketEndpointLockManager::GetInstance()->UnlockEndpoint(
On 2014/06/18 06:49:42, tyoshino wrote:
> don't we call UnlockEndpoint() twice when DoTransportConnectComplete() is
called
> with non-OK result?

No, because next_state_ is set to STATE_NONE in DoLoop() before
DoTransportConnectComplete() is called. I added an explicit assignment to
next_state_ in DoTransportConnectComplete() to try to make this less confusing.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:155: STATE_WAIT_FOR_LOCK,
On 2014/06/18 06:49:43, tyoshino wrote:
> [optional] wondering if s/WAIT/OBTAIN/ sounds better or not

Sounds good to me. Most of the time we don't actually need to wait. Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:186: case
STATE_WAIT_FOR_LOCK:
On 2014/06/18 06:49:43, tyoshino wrote:
> DCHECK_EQ(OK, rv)

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:189: case
STATE_WAIT_FOR_LOCK_COMPLETE:
On 2014/06/18 06:49:42, tyoshino wrote:
> DCHECK_EQ(OK, rv)

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:253: return result;
On 2014/06/16 23:41:11, Johnny wrote:
> Sanity check: Looks like WebSocketBasicHandshakeStream::Upgrade() is
responsible
> for releasing the lock on success, allowing a new connection to start?

Yes.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:360: //
LOAD_STATE_CONNECTING is better than
On 2014/06/18 06:49:43, tyoshino wrote:
> could you please elaborate better for what point

I updated the comment. PTAL.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:395: DVLOG(1) <<
"Unexpected ADDRESS_FAMILY: " << it->GetFamily();
On 2014/06/18 06:49:43, tyoshino wrote:
> do we want to ignore this?

At the moment this cannot happen, so we could put NOTREACHED() here. But this
class is not closely coupled to the host resolver so I don't want to make strong
assertions about its behaviour.

If we got passed something that was neither IPv4 nor IPv6 we wouldn't know what
to do with it anyway. If we don't recognise the family of any of the addresses,
we will fall through and return ERR_UNEXPECTED, which seems reasonable.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:418: // This use of
base::Unretained is safe because fallback_timer_ is owned
On 2014/06/18 06:49:42, tyoshino wrote:
> |fallback_timer_|

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:426: }
On 2014/06/18 06:49:43, tyoshino wrote:
> how about breaking L417 into two if-s and return when result == ERR_IO_PENDING
> && !ipv4_job_? and then, you can use switch for L409, L417 and L427.
> 
> i understand that this code works but i guess it improves readability
> 
> it's optional

Very clever. Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:574:
TimeDelta::FromSeconds(kTransportConnectJobTimeoutInSeconds),
On 2014/06/18 06:49:42, tyoshino wrote:
> ConnectionTimeout()?

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:724: if (!handed_out_socket
&& is_stalled_ && !ReachedMaxSocketsLimit())
On 2014/06/18 06:49:43, tyoshino wrote:
> is this correct? pending_connects_.size() decreases at L727

No, it's very wrong. I didn't write any code to restart stalled sockets. Fixed.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:769:
handle->set_idle_time(TimeDelta());
On 2014/06/18 06:49:43, tyoshino wrote:
> needed?

No. Replaced with DCHECKs.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:17: #include
"base/timer/timer.h"
On 2014/06/18 06:49:43, tyoshino wrote:
> include net/base/net_log.h as you have BoundNetLog in
> WebSocketTransportConnectJob

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:30:
OnHostResolutionCallback;
On 2014/06/18 06:49:43, tyoshino wrote:
> remove and use TransportClientSocketPool's one in the unittest?

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:39: // timer
(kIPv6FallbackTimerInMs) and start a connect() to a IPv4 address if the
On 2014/06/18 06:49:43, tyoshino wrote:
> an IPv4?

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:88: // Called from
fallback_timer_
On 2014/06/16 23:41:11, Johnny wrote:
> Nit: Called from |fallback_timer_|.

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:96:
TransportConnectJobHelper data_;
On 2014/06/16 23:41:11, Johnny wrote:
> Optional: |data_| => |helper_|.

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.h:100: // go first, followed
after |kIPv6FallbackTimerInMs| by the IPV4
On 2014/06/18 06:49:43, tyoshino wrote:
> IPv4

Done.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:91: class
MockClientSocket : public StreamSocket {
On 2014/06/16 23:41:11, Johnny wrote:
> Do these mocks deviate significantly from transport_client_socket_unittest.cc?
> Can they share implementation?

MockClientSocket has gained an implementation of GetPeerAddress().
MockFailedClientSocket is unchanged. MockTriggerableClientSocket replaces
MockPendingClientSocket but keeps the same functionality. I will share the
implementations.

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:488: virtual
~WebSocketTransportClientSocketPoolTest() {}
On 2014/06/16 23:41:11, Johnny wrote:
> Verify that WebSocketEndpointLockManager is empty/in an expected state?
> 
> Also, I don't think I saw tests specifically on WebSocketEndpointLockManager.

Done.

https://codereview.chromium.org/240873003/diff/320001/net/websockets/websocke...
File net/websockets/websocket_stream_test.cc (right):

https://codereview.chromium.org/240873003/diff/320001/net/websockets/websocke...
net/websockets/websocket_stream_test.cc:89: ->CloseAllConnections();
On 2014/06/18 06:49:43, tyoshino wrote:
> please write the reason why you need this

I thought I needed it to make sure all the mock sockets were destroyed after
each test case, but I was confused. I have removed it.

[Adam Rice, 2014-06-19 13:56:20]

-eroman, +darin for base::LinkedList<> changes OWNER approval.

[tyoshino (SeeGerritForStatus), 2014-06-20 12:12:07]

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:360: //
LOAD_STATE_CONNECTING is better than
On 2014/06/19 13:55:20, Adam Rice wrote:
> On 2014/06/18 06:49:43, tyoshino wrote:
> > could you please elaborate better for what point
> 
> I updated the comment. PTAL.

Thanks

https://codereview.chromium.org/240873003/diff/320001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:395: DVLOG(1) <<
"Unexpected ADDRESS_FAMILY: " << it->GetFamily();
On 2014/06/19 13:55:20, Adam Rice wrote:
> On 2014/06/18 06:49:43, tyoshino wrote:
> > do we want to ignore this?
> 
> At the moment this cannot happen, so we could put NOTREACHED() here. But this
> class is not closely coupled to the host resolver so I don't want to make
strong
> assertions about its behaviour.
> 
> If we got passed something that was neither IPv4 nor IPv6 we wouldn't know
what
> to do with it anyway. If we don't recognise the family of any of the
addresses,
> we will fall through and return ERR_UNEXPECTED, which seems reasonable.

OK

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
File net/socket/websocket_endpoint_lock_manager.h (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
net/socket/websocket_endpoint_lock_manager.h:57: // Checks that
|endpoint_job_map_| and |socket_endpoint_map_| are empty. For
job -> waiter

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:8: #include <limits>
remove

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:9: #include <map>
remove

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:10: #include <utility>
remove

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:14: #include
"base/memory/singleton.h"
remove

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:16: #include
"base/stl_util.h"
remove

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:20: #include
"net/base/ip_endpoint.h"
can be removed?

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:298:
StalledRequestMap::value_type(handle, iterator));
please write why we can use the std::list iterator later briefly

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
File net/socket/websocket_transport_connect_sub_job.cc (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_connect_sub_job.cc:146: next_state_ = STATE_NONE;
thanks. but is it ok to make started() return false after
DoTransportConnectComplete?

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
File net/socket/websocket_transport_connect_sub_job.h (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_connect_sub_job.h:9: #include
"base/containers/linked_list.h"
remove

[Johnny, 2014-06-20 21:32:58]

This looks good to me. My only remaining question is whether there are potential
re-entrance issues within WSELM::UnlockEndpoint()'s notification of a pending
job, and in the tear-down of WSTCSP. See inline comments.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:256:
WebSocketTransportClientSocketPool::~WebSocketTransportClientSocketPool() {
I think I'm surprised by these dcheck's. What's the basis for asserting these
are empty? What if, eg, the HttpNetworkSession is going away?

Specifically I'm comparing to ~ClientSocketPoolBaseHelper, which does destroy
outstanding jobs:
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/client_...

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:380: void
WebSocketTransportClientSocketPool::FlushWithError(int error) {
Are there re-entrance concerns with these loops? Eg a deleted
WebSocketTransportConnectJob implicitly deletes it's
WebSocketTransportConnectSubJobs, which in turn calls WSELM::Unlock(), which
unblocks a new job, which then attempts to connect and fails synchronously, and
then attempts to remove itself mid-iteration? And/or notifies it's delegate,
which retries, and inserts a new job mid-iteration?

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:549: delete it->second,
it->second = NULL;
Could |it| be invalidated here?

[darin (slow to review), 2014-06-21 04:04:06]

src/base/ changes LGTM

[tyoshino (SeeGerritForStatus), 2014-06-23 09:31:36]

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
File net/socket/websocket_endpoint_lock_manager_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
net/socket/websocket_endpoint_lock_manager_unittest.cc:6: 
include net_errors.h

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
net/socket/websocket_endpoint_lock_manager_unittest.cc:16: // A SocketStream
implementation with no functionality at all.
StreamSocket

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
net/socket/websocket_endpoint_lock_manager_unittest.cc:23: // SocketStream
implementation
StreamSocket

[Adam Rice, 2014-06-23 10:06:27]

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
File net/socket/websocket_endpoint_lock_manager.h (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
net/socket/websocket_endpoint_lock_manager.h:57: // Checks that
|endpoint_job_map_| and |socket_endpoint_map_| are empty. For
On 2014/06/20 12:12:06, tyoshino wrote:
> job -> waiter

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool.cc (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:8: #include <limits>
On 2014/06/20 12:12:07, tyoshino wrote:
> remove

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:9: #include <map>
On 2014/06/20 12:12:07, tyoshino wrote:
> remove

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:10: #include <utility>
On 2014/06/20 12:12:07, tyoshino wrote:
> remove

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:14: #include
"base/memory/singleton.h"
On 2014/06/20 12:12:07, tyoshino wrote:
> remove

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:16: #include
"base/stl_util.h"
On 2014/06/20 12:12:06, tyoshino wrote:
> remove

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:20: #include
"net/base/ip_endpoint.h"
On 2014/06/20 12:12:06, tyoshino wrote:
> can be removed?

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:256:
WebSocketTransportClientSocketPool::~WebSocketTransportClientSocketPool() {
On 2014/06/20 21:32:58, Johnny wrote:
> I think I'm surprised by these dcheck's. What's the basis for asserting these
> are empty? What if, eg, the HttpNetworkSession is going away?
> 
> Specifically I'm comparing to ~ClientSocketPoolBaseHelper, which does destroy
> outstanding jobs:
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/client_...

I'm not completely sure about this, since unlike the other pools, this pool uses
early binding of handles to jobs, and so it may be safe to destroy without any
additional cleanup. However, I have added a FlushWithError() anyway, to be on
the safe side.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:298:
StalledRequestMap::value_type(handle, iterator));
On 2014/06/20 12:12:07, tyoshino wrote:
> please write why we can use the std::list iterator later briefly

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:380: void
WebSocketTransportClientSocketPool::FlushWithError(int error) {
On 2014/06/20 21:32:58, Johnny wrote:
> Are there re-entrance concerns with these loops? Eg a deleted
> WebSocketTransportConnectJob implicitly deletes it's
> WebSocketTransportConnectSubJobs, which in turn calls WSELM::Unlock(), which
> unblocks a new job, which then attempts to connect and fails synchronously,
and
> then attempts to remove itself mid-iteration? And/or notifies it's delegate,
> which retries, and inserts a new job mid-iteration?

Well spotted! I have added tests and a flag to guard against re-entrancy issues.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool.cc:549: delete it->second,
it->second = NULL;
On 2014/06/20 21:32:58, Johnny wrote:
> Could |it| be invalidated here?

No. I have added a comment and a DCHECK to clarify and verify.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
File net/socket/websocket_transport_connect_sub_job.cc (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_connect_sub_job.cc:146: next_state_ = STATE_NONE;
On 2014/06/20 12:12:07, tyoshino wrote:
> thanks. but is it ok to make started() return false after
> DoTransportConnectComplete?

Currently it is, because we always exit synchronously after completion and
started() only has one caller, but I added a STATE_DONE just to make it robust
against future changes.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
File net/socket/websocket_transport_connect_sub_job.h (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_tr...
net/socket/websocket_transport_connect_sub_job.h:9: #include
"base/containers/linked_list.h"
On 2014/06/20 12:12:07, tyoshino wrote:
> remove

Done.

[Johnny, 2014-06-23 16:10:00]

LGTM for net

[Adam Rice, 2014-06-24 02:12:08]

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
File net/socket/websocket_endpoint_lock_manager_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
net/socket/websocket_endpoint_lock_manager_unittest.cc:6: 
On 2014/06/23 09:31:35, tyoshino wrote:
> include net_errors.h

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
net/socket/websocket_endpoint_lock_manager_unittest.cc:16: // A SocketStream
implementation with no functionality at all.
On 2014/06/23 09:31:36, tyoshino wrote:
> StreamSocket

Done.

https://codereview.chromium.org/240873003/diff/360001/net/socket/websocket_en...
net/socket/websocket_endpoint_lock_manager_unittest.cc:23: // SocketStream
implementation
On 2014/06/23 09:31:35, tyoshino wrote:
> StreamSocket

Done.

[tyoshino (SeeGerritForStatus), 2014-06-24 05:46:53]

lgtm

https://codereview.chromium.org/240873003/diff/380001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool_test_util.cc (right):

https://codereview.chromium.org/240873003/diff/380001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_test_util.cc:9: #include
"base/run_loop.h"
include base/logging.h

https://codereview.chromium.org/240873003/diff/380001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_test_util.cc:13: #include
"net/base/net_util.h"
include base/memory/weak_ptr.h

https://codereview.chromium.org/240873003/diff/380001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/380001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:228: // Rest of
them wait foe the first socket to be released.
foe -> for

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:517: // explicity
released.
explicitly

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:543:
pool_.CancelRequest("a", (request(0))->handle());
parentheses needed?

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:566: // Resolve an
AddressList with a IPv6 address first and then a IPv4 address.
an IPv6
an IPv4

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:609: // Resolve an
AddressList with a IPv6 address first and then a IPv4 address.
an IPv6
an IPv4

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:874: // Resolve an
AddressList with 6 IPv6 addresses and 6 IPv4 address.
addresses

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:1008:
client_socket_factory_.set_client_socket_type(
you might want to name this set_default_client_socket_type. but let's do it in
another CL if you agree.

[Adam Rice, 2014-06-24 06:28:38]

https://codereview.chromium.org/240873003/diff/380001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool_test_util.cc (right):

https://codereview.chromium.org/240873003/diff/380001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_test_util.cc:9: #include
"base/run_loop.h"
On 2014/06/24 05:46:52, tyoshino wrote:
> include base/logging.h

Done.

https://codereview.chromium.org/240873003/diff/380001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_test_util.cc:13: #include
"net/base/net_util.h"
On 2014/06/24 05:46:52, tyoshino wrote:
> include base/memory/weak_ptr.h

Done.

https://codereview.chromium.org/240873003/diff/380001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/380001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:228: // Rest of
them wait foe the first socket to be released.
On 2014/06/24 05:46:52, tyoshino wrote:
> foe -> for

Done.

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:517: // explicity
released.
On 2014/06/24 05:46:52, tyoshino wrote:
> explicitly

Done.

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:543:
pool_.CancelRequest("a", (request(0))->handle());
On 2014/06/24 05:46:53, tyoshino wrote:
> parentheses needed?

No. Removed.

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:566: // Resolve an
AddressList with a IPv6 address first and then a IPv4 address.
On 2014/06/24 05:46:52, tyoshino wrote:
> an IPv6
> an IPv4

Done.

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:609: // Resolve an
AddressList with a IPv6 address first and then a IPv4 address.
On 2014/06/24 05:46:53, tyoshino wrote:
> an IPv6
> an IPv4

Done.

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:874: // Resolve an
AddressList with 6 IPv6 addresses and 6 IPv4 address.
On 2014/06/24 05:46:53, tyoshino wrote:
> addresses

Done.

https://codereview.chromium.org/240873003/diff/400001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:1008:
client_socket_factory_.set_client_socket_type(
On 2014/06/24 05:46:53, tyoshino wrote:
> you might want to name this set_default_client_socket_type. but let's do it in
> another CL if you agree.

That makes sense. But it affects the TransportClientSocketPool unit tests as
well, so a separate CL is better.

[Adam Rice, 2014-06-24 09:52:47]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2014-06-24 09:53:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ricea@chromium.org/240873003/480001

[commit-bot: I haz the power, 2014-06-24 12:54:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-06-24 13:28:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-24 13:28:30]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[Adam Rice, 2014-06-24 22:47:15]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2014-06-24 22:49:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ricea@chromium.org/240873003/480001

[commit-bot: I haz the power, 2014-06-24 22:53:36]

Change committed as 279524

[Ryan Sleevi, 2014-07-08 18:50:03]

Hi Adam,

I was alerted to this change earlier, and I apologize for not noticing it
sooner. I would have pushed back on landing this, as I think it's doing several
things we try very hard not to do in //net.

While I realize it's no meetings period, and I also understand there are some
timezone difficulties, I think it would be good if we had a chance at some point
to discuss design.

I realize that now that this code has landed (and there may be more code
depending on it now), it's going to be harder to refactor. However, I think the
"Helper" here is very dangerous for encapsulation of the pools, and will make
the code harder to understand, so I'd very much like to push back on it and see
if we can't come up with an alternate design that has a stronger separation of
responsibility and isolation.

We can chat more over email / work on a design doc, but it'd be good to make
sure I understand the problem space. That said, I've highlighted some concerns
that I think are fairly serious below.

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:88: 
This is a huge set of methods to leave public, for any #include'r of
TransportClientSocketPool.

This seems to break multiple layers of encapsulation/isolation - e.g. - these
details are not exposed to holders of TransportConnectJobs normally.

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:98: int DoConnectInternal(T* job);
These methods are public.

Anyone with a helper can use it to fully mutate the state of an Job. That's
definitely a layering violation.

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:104: void OnIOComplete(T* job, int
result);
Template methods like this are almost always a bad thing - they accept any type
and can be used to mutate the internal state.

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:161: friend class
TransportConnectJobHelper;
Every time we have a friend class, we have failed encapsulation, and it should
only be done with great pain/suffering/introspection.

I'd like to work to explore alternate designs that actually maintain a
well-encapsulated boundary.

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:242: protected:
The Chromium style guide strongly discourages protected methods

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:246: const
scoped_refptr<TransportSocketParams>* casted_params);
This doesn't look right at all. A pointer to a const scoped-refptr?

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool_test_util.h (right):

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_test_util.h:67: };
We already have utilities in Chromium to simulate these - the MockConnect
interfaces (and friends).

I'd like to better understand why this is necessary, as it seems like it is
reinventing similar code to our deterministic sockets (and friends)

https://codereview.chromium.org/240873003/diff/480001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/480001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:1055:
TransportConnectJobHelper::kIPv6FallbackTimerInMs));
This is very bad.

We try very hard to avoid hardcoded timeouts in tests. We should explore
alternate designs.

[tyoshino (SeeGerritForStatus), 2014-07-09 00:17:40]

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:88: 
Sorry about this. I saw use of ClientSocketPoolBaseHelper and was taking things
easy too much.

Basically they're for reducing code duplication. We should be able to put them
into each pool implementation as a quick fix while we should also try to come up
with better design with less code duplication.

Also, shall we put this into an internal namespace as well as
ClientSocketPoolBaseHelper?

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool_test_util.h (right):

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool_test_util.h:67: };
On 2014/07/08 18:50:03, Ryan Sleevi wrote:
> We already have utilities in Chromium to simulate these - the MockConnect
> interfaces (and friends).
> 
> I'd like to better understand why this is necessary, as it seems like it is
> reinventing similar code to our deterministic sockets (and friends)

FYI, this is refactored version of MockClientSocketFactory which
transport_client_socket_pool_unittest.cc had. Of course, it'll be nice to
replace usage of this with MockConnect if possible.

[Ryan Sleevi, 2014-07-09 00:20:46]

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:88: 
On 2014/07/09 00:17:40, tyoshino wrote:
> Sorry about this. I saw use of ClientSocketPoolBaseHelper and was taking
things
> easy too much.

Right, CSPBH is to be a base class for a set of templated classes, but avoid the
overhead of instantiating all of the common methods on the templates.

That is, it's a performance optimization, more than a code/dependency
optimization.

> 
> Basically they're for reducing code duplication. We should be able to put them
> into each pool implementation as a quick fix while we should also try to come
up
> with better design with less code duplication.

Right, I think we're on the same page that we want to minimize code duplication
(through template instantiation) and code duplication (through
implementation/copy paste). We just want to make sure none of that
de-duplication leaks out into the interfaces, nor does it violate the layering.

> 
> Also, shall we put this into an internal namespace as well as
> ClientSocketPoolBaseHelper?

Yes, this is something we'd minimally mark internal

[Adam Rice, 2014-07-09 08:33:03]

Ryan,

Thank you for your review. I am happy to meet with you to discuss the design.
Feel free to drop something on my calendar--I can easily join a VC from home.

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
File net/socket/transport_client_socket_pool.h (right):

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:98: int DoConnectInternal(T* job);
On 2014/07/08 18:50:02, Ryan Sleevi wrote:
> These methods are public.
> 
> Anyone with a helper can use it to fully mutate the state of an Job. That's
> definitely a layering violation.

Sorry, yes, when I wrote this I was just focussed on factoring out the common
code in the simplest way possible. I was expecting to get some push-back that
would ultimately lead to a better solution, so I wasn't paying attention to
encapsulation.

But it's easy to miss things in a big CL, especially one with parts that are
ugly-as-designed, so this ended-up getting checked in as-is.

https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
net/socket/transport_client_socket_pool.h:246: const
scoped_refptr<TransportSocketParams>* casted_params);
On 2014/07/08 18:50:02, Ryan Sleevi wrote:
> This doesn't look right at all. A pointer to a const scoped-refptr?

Whereas the void* argument to RequestSocket() is totally cool?

https://codereview.chromium.org/240873003/diff/480001/net/socket/websocket_tr...
File net/socket/websocket_transport_client_socket_pool_unittest.cc (right):

https://codereview.chromium.org/240873003/diff/480001/net/socket/websocket_tr...
net/socket/websocket_transport_client_socket_pool_unittest.cc:1055:
TransportConnectJobHelper::kIPv6FallbackTimerInMs));
On 2014/07/08 18:50:03, Ryan Sleevi wrote:
> This is very bad.
> 
> We try very hard to avoid hardcoded timeouts in tests. We should explore
> alternate designs.

https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/client_...

[Ryan Sleevi, 2014-07-09 22:48:07]

On 2014/07/09 08:33:03, Adam Rice wrote:
> Ryan,
> 
> Thank you for your review. I am happy to meet with you to discuss the design.
> Feel free to drop something on my calendar--I can easily join a VC from home.

Will do. I do want to make sure I understand this code better, so we can discuss
possible solutions.

> 
>
https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
> net/socket/transport_client_socket_pool.h:246: const
> scoped_refptr<TransportSocketParams>* casted_params);
> On 2014/07/08 18:50:02, Ryan Sleevi wrote:
> > This doesn't look right at all. A pointer to a const scoped-refptr?
> 
> Whereas the void* argument to RequestSocket() is totally cool?

I'm not sure your point? We should always strive to do the best, independent of
what other code.

(And no, it's not ideal, but it's an entirely separate rationale. I'm solely
looking at this code in isolation)

> On 2014/07/08 18:50:03, Ryan Sleevi wrote:
> > This is very bad.
> > 
> > We try very hard to avoid hardcoded timeouts in tests. We should explore
> > alternate designs.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/socket/client_...

I'm not sure your point of mentioning this code. Is it "Someone else did
something bad, so it's OK for me to do something bad"? If so, that's not a very
healthy attitude for maintaining code health, nor does it change the concern I
raised.

There's also a difference between 1ms and (>1ms). We've slowly been working to
reduce the number of timeouts - we used to have tests that would wait 5
seconds(!!!) for timeouts, which are even worse.

[Adam Rice, 2014-07-10 03:35:21]

On 2014/07/09 22:48:07, Ryan Sleevi wrote:
>
https://codereview.chromium.org/240873003/diff/480001/net/socket/transport_cl...
> > net/socket/transport_client_socket_pool.h:246: const
> > scoped_refptr<TransportSocketParams>* casted_params);
> > On 2014/07/08 18:50:02, Ryan Sleevi wrote:
> > > This doesn't look right at all. A pointer to a const scoped-refptr?
> > 
> > Whereas the void* argument to RequestSocket() is totally cool?
> 
> I'm not sure your point? We should always strive to do the best, independent
of
> what other code.

The point is that you look at that type and say "wtf?". Which is the correct
response. But it's the same type that is being passed into RequestSocket(),
disguised as void*. By wearing its ugliness on its sleeve, it is intended to
help the reader understand what is going on.

> (And no, it's not ideal, but it's an entirely separate rationale. I'm solely
> looking at this code in isolation)

This CL has an explicit design goal of not damaging the readability or
performance
of the existing code. Looking at it in isolation it is impossible to see how far
it
falls short of that goal.