Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(117)

Issue 1096203006: Collect all ConnectionAttempts from both sockets in TransportConnectJob. (Closed)

Created:
5 years, 6 months ago by Deprecated (see juliatuttle)
Modified:
5 years, 5 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@domrel_serverip1
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Collect all ConnectionAttempts from both sockets in TransportConnectJob. Before, the TransportConnectJob simply inferred that, if the main socket failed to connect, the address it was using was the last address in the list. With this change, the TCPClientSocket actually tracks all of the connection attempts made (as it tries each address in the list), and the TransportConnectJob copies the attempts from both the main and fallback sockets and records all of them in the ClientSocketHandle in GetAdditionalErrorState. BUG=480565 TBR=jam Committed: https://crrev.com/23fdb7b4ed86ee9f63b16a3b432fa2c721432bd2 Cr-Commit-Position: refs/heads/master@{#330012}

Patch Set 1 : rebase #

Total comments: 11

Patch Set 2 : rebase #

Patch Set 3 : Remove obsolete TODO; try to fix StreamSocket build errors #

Patch Set 4 : rebase #

Patch Set 5 : Copy attempts from both sockets when either succeeds #

Patch Set 6 : rebase #

Patch Set 7 : Record all attempts from both sockets; clear attempts on reuse #

Patch Set 8 : rebase #

Total comments: 14

Patch Set 9 : Document why Do*ConnectComplete don't delete other socket #

Total comments: 17

Patch Set 10 : Make requested changes; rebase #

Total comments: 4

Patch Set 11 : Fix compile errors #

Patch Set 12 : And another #

Patch Set 13 : rebase #

Patch Set 14 : ...and another #

Patch Set 15 : Flesh out unittests. #

Patch Set 16 : Add a couple comments #

Total comments: 9

Patch Set 17 : Make requested changes #

Patch Set 18 : Fix one more compile error #

Patch Set 19 : Fix StreamSocket destructor #

Patch Set 20 : No, really #

Patch Set 21 : Return fake ConnectionAttempt in MockTCPClientSocket #

Unified diffs Side-by-side diffs Delta from patch set Stats (+392 lines, -32 lines) Patch
M chrome/browser/devtools/device/usb/android_usb_socket.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/devtools/device/usb/android_usb_socket.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/socket/tls_socket_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/renderer_host/p2p/socket_host_test_utils.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +4 lines, -0 lines 0 comments Download
M content/browser/renderer_host/p2p/socket_host_test_utils.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +4 lines, -0 lines 0 comments Download
M jingle/glue/fake_ssl_client_socket.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M jingle/glue/fake_ssl_client_socket.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M jingle/glue/fake_ssl_client_socket_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M jingle/glue/proxy_resolving_client_socket.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +4 lines, -0 lines 0 comments Download
M jingle/glue/proxy_resolving_client_socket.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M jingle/glue/pseudotcp_adapter.h View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M jingle/glue/pseudotcp_adapter.cc View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download
M net/http/http_proxy_client_socket.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/http/http_proxy_client_socket.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M net/http/http_stream_factory_impl_job.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M net/http/http_stream_factory_impl_job.cc View 1 2 3 4 5 6 7 8 9 6 chunks +18 lines, -7 lines 0 comments Download
M net/server/http_server_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M net/socket/client_socket_handle.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/client_socket_pool_base.cc View 1 2 3 4 5 6 1 chunk +6 lines, -0 lines 0 comments Download
M net/socket/client_socket_pool_base_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M net/socket/connection_attempts.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +2 lines, -0 lines 0 comments Download
M net/socket/socket_test_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +6 lines, -0 lines 0 comments Download
M net/socket/socket_test_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 2 chunks +20 lines, -0 lines 0 comments Download
M net/socket/socks5_client_socket.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/socks5_client_socket.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/socks_client_socket.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/socks_client_socket.cc View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_nss.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_nss.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +4 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_openssl.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_openssl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +5 lines, -0 lines 0 comments Download
M net/socket/ssl_client_socket_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +9 lines, -0 lines 0 comments Download
M net/socket/ssl_server_socket_nss.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/ssl_server_socket_nss.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M net/socket/ssl_server_socket_openssl.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/ssl_server_socket_openssl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +5 lines, -0 lines 0 comments Download
M net/socket/ssl_server_socket_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -0 lines 0 comments Download
M net/socket/stream_socket.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +11 lines, -0 lines 0 comments Download
M net/socket/tcp_client_socket.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +8 lines, -0 lines 0 comments Download
M net/socket/tcp_client_socket.cc View 1 2 3 4 5 6 3 chunks +18 lines, -0 lines 0 comments Download
M net/socket/transport_client_socket_pool.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +11 lines, -1 line 0 comments Download
M net/socket/transport_client_socket_pool.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 7 chunks +50 lines, -10 lines 0 comments Download
M net/socket/transport_client_socket_pool_test_util.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/transport_client_socket_pool_test_util.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 6 chunks +34 lines, -3 lines 0 comments Download
M net/socket/transport_client_socket_pool_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 8 chunks +33 lines, -10 lines 0 comments Download
M net/socket/unix_domain_client_socket_posix.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/socket/unix_domain_client_socket_posix.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M net/socket/websocket_endpoint_lock_manager_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +8 lines, -0 lines 0 comments Download
M net/spdy/spdy_proxy_client_socket.h View 1 2 3 4 5 6 7 8 9 1 chunk +3 lines, -0 lines 0 comments Download
M net/spdy/spdy_proxy_client_socket.cc View 1 2 3 4 5 6 7 8 9 1 chunk +5 lines, -0 lines 0 comments Download
M remoting/protocol/channel_multiplexer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +6 lines, -0 lines 0 comments Download
M remoting/protocol/fake_stream_socket.h View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +4 lines, -0 lines 0 comments Download
M remoting/protocol/fake_stream_socket.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 31 (13 generated)
Deprecated (see juliatuttle)
PTAL when you get a chance; note dependency in CL description.
5 years, 6 months ago (2015-04-24 02:34:33 UTC) #2
Randy Smith (Not in Mondays)
High level comments: * Bug #? * Remove dependency information from CL description. * I'm ...
5 years, 5 months ago (2015-05-01 17:39:50 UTC) #6
Deprecated (see juliatuttle)
PTAL. https://codereview.chromium.org/1096203006/diff/80001/net/socket/stream_socket.h File net/socket/stream_socket.h (right): https://codereview.chromium.org/1096203006/diff/80001/net/socket/stream_socket.h#newcode9 net/socket/stream_socket.h:9: #include "net/socket/connection_attempts.h" On 2015/05/01 17:39:49, rdsmith wrote: > ...
5 years, 5 months ago (2015-05-04 19:44:50 UTC) #8
Randy Smith (Not in Mondays)
https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_client_socket_pool.cc File net/socket/transport_client_socket_pool.cc (left): https://codereview.chromium.org/1096203006/diff/80001/net/socket/transport_client_socket_pool.cc#oldcode248 net/socket/transport_client_socket_pool.cc:248: ConnectionAttempt(helper_.addresses().back(), connect_result_)); On 2015/05/04 19:44:50, ttuttle wrote: > On ...
5 years, 5 months ago (2015-05-06 15:30:12 UTC) #9
Deprecated (see juliatuttle)
Okay, here's a new version: 1. It makes sure all of the attempts make it ...
5 years, 5 months ago (2015-05-08 23:31:52 UTC) #10
Randy Smith (Not in Mondays)
I'm afraid you're getting a lot of comments in proportion to the amount of effort ...
5 years, 5 months ago (2015-05-12 20:11:09 UTC) #11
Deprecated (see juliatuttle)
https://codereview.chromium.org/1096203006/diff/220001/net/http/http_stream_factory_impl_job.h File net/http/http_stream_factory_impl_job.h (right): https://codereview.chromium.org/1096203006/diff/220001/net/http/http_stream_factory_impl_job.h#newcode283 net/http/http_stream_factory_impl_job.h:283: void MaybeCopyConnectionAttempts(); On 2015/05/12 20:11:09, rdsmith wrote: > I ...
5 years, 5 months ago (2015-05-13 18:22:29 UTC) #12
Randy Smith (Not in Mondays)
https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_socket.h File net/socket/tcp_client_socket.h (right): https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_socket.h#newcode124 net/socket/tcp_client_socket.h:124: ConnectionAttempts connection_attempts_; On 2015/05/13 18:22:29, ttuttle wrote: > On ...
5 years, 5 months ago (2015-05-14 19:42:05 UTC) #13
Deprecated (see juliatuttle)
PTAL. https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_socket.h File net/socket/tcp_client_socket.h (right): https://codereview.chromium.org/1096203006/diff/240001/net/socket/tcp_client_socket.h#newcode124 net/socket/tcp_client_socket.h:124: ConnectionAttempts connection_attempts_; On 2015/05/14 19:42:04, rdsmith wrote: > ...
5 years, 5 months ago (2015-05-14 20:02:35 UTC) #14
Randy Smith (Not in Mondays)
I can't believe I/we ate the whole thing. LGTM. https://codereview.chromium.org/1096203006/diff/380001/net/socket/transport_client_socket_pool.cc File net/socket/transport_client_socket_pool.cc (right): https://codereview.chromium.org/1096203006/diff/380001/net/socket/transport_client_socket_pool.cc#newcode480 net/socket/transport_client_socket_pool.cc:480: ...
5 years, 5 months ago (2015-05-14 20:29:55 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096203006/440001
5 years, 5 months ago (2015-05-14 20:37:35 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_chromeos_rel/builds/37805) linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, ...
5 years, 5 months ago (2015-05-14 20:44:56 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096203006/420002
5 years, 5 months ago (2015-05-14 20:47:27 UTC) #23
commit-bot: I haz the power
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/25152)
5 years, 5 months ago (2015-05-14 21:32:50 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1096203006/470001
5 years, 5 months ago (2015-05-15 00:28:02 UTC) #28
commit-bot: I haz the power
Committed patchset #21 (id:470001)
5 years, 5 months ago (2015-05-15 01:28:12 UTC) #29
commit-bot: I haz the power
Patchset 21 (id:??) landed as https://crrev.com/23fdb7b4ed86ee9f63b16a3b432fa2c721432bd2 Cr-Commit-Position: refs/heads/master@{#330012}
5 years, 5 months ago (2015-05-15 01:29:09 UTC) #30
Randy Smith (Not in Mondays)
5 years, 5 months ago (2015-05-18 15:12:38 UTC) #31
Message was sent while issue was closed.
PS21 LGTM.

Powered by Google App Engine
This is Rietveld 408576698