H2 proxies break socket pool isolation. |
|||
Issue descriptionH2 proxies create an entry in the global H2 session pool for a server that uses direct connections, and sits on top of the H2 proxies socket pools. H2 direct connections *also* add an entry for a server that uses direct connections, but uses the root socket pool. This means we're potentially mixing and matching socket pools here. For instance, if we create a direct H2 connection to foo.com, we create a socket in the global socket pool, which contributes to the global socket pool limit. If we then use foo.com as an HTTPS proxy (As a tunnel - so for an HTTPS/H2 connection), we'll reuse that for a proxy connection. If we then tell the root socket pool to close an idle connection, it will bubble up the request to the main socket pool (Which the main H2 connection belongs to), and see that connection as used, and give up. It won't make its way up to the proxy client socket wrapper class, which may be idle, because that object is in the proxy's socket pool, which is supposed to be entirely disjoint from the main socket pool. I'm not entirely sure if the opposite case also creates problems (Direct H2 requests on a socket that lives in the proxy socket pool). Regardless, the current design seems a bit concerning here as well. We should figure this out before we hook up QUIC tunnels.
,
Oct 15
I thought that the key to the "H2 Pool", aka the SpdySessionPool included the proxy server:
// SpdySessionKey is used as unique index for SpdySessionPool.
class NET_EXPORT_PRIVATE SpdySessionKey {
public:
SpdySessionKey();
SpdySessionKey(const HostPortPair& host_port_pair,
const ProxyServer& proxy_server,
PrivacyMode privacy_mode,
const SocketTag& socket_tag);
As such, I *thought* that a direct connection to foo.com and a connection to foo.com as a proxy would be two different SpdySession objects. But maybe that's not the case? Do we have test coverage for this?
> We should figure this out before we hook up QUIC tunnels.
FWIW, I believe we already have this hooked up.
,
Oct 15
Connections to the proxy server itself use a Direct ProxyServer. See https://cs.chromium.org/chromium/src/net/http/http_proxy_client_socket_wrapper.cc?q=proxy.*wrapper&sq=package:chromium&g=0&l=595
,
Oct 15
I haven't made a unit test for this behavior, so I could be mistaken, but I'm relatively confident that this is a real bug.
,
Oct 15
For the proof of concept: Make an HTTPS request to https://proxy.com, using no proxy. Get an H2 connection and a response. The socket is in the main socket pool. Makes an HTTPS request to https://foo.com, using proxy.com as an HTTPS connection. This reuses the original H2 connection, and creates a proxy socket on top of it, in the proxy socket pool (Which is completely detached socket pool hierarchy from the main socket pool). Wait for a successful response. Call CloseAllIdleConnections() (Not sure if that's the real name) on the main socket pool. Nothing will happen, because it sees the idle HTTPS tunneled socket as used. Only the proxy client socket pool sees it as idle, but that's not layered on top of the main pool. Call CloseAllIdleConnections() on the proxy pool. Now the tunnel is actually closed. Call CloseAllIdleConnections() on the main socket pool. Now the H2 session is really closed. This matters in a couple cases - when closing all sockets on network change, obviously, but also when trying to close an idle socket to service a new socket request in the main socket pool.
,
Oct 15
> Connections to the proxy server itself use a Direct ProxyServer Oh, duh! Of course. I'm sorry for adding chaff to the bug.
,
Dec 4
Going to fix this before I start my socket pool refactor, though I'll need to refactor this a bit again once I've flattened the pools up to the HTTP proxy layer.
,
Dec 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2436b2f13d17a72091143b0041a74d586f584308 commit 2436b2f13d17a72091143b0041a74d586f584308 Author: Matt Menke <mmenke@chromium.org> Date: Tue Dec 11 18:07:11 2018 Don't pool HTTP2 sessions used for tunnels and direct connections. Pooling sockets from independent socket pools together breaks layering assumptions. A later point in time, these pools will also need to wrap lower layer objects than ClientSocketHandles. Bug: 895562 Change-Id: I8235f244048475bcb64bf48b3e15309e8607e85e Reviewed-on: https://chromium-review.googlesource.com/c/1362192 Commit-Queue: Matt Menke <mmenke@chromium.org> Reviewed-by: Bence Béky <bnc@chromium.org> Cr-Commit-Position: refs/heads/master@{#615585} [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/http/bidirectional_stream_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/http/http_network_transaction_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/http/http_proxy_client_socket_wrapper.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/http/http_stream_factory_job.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/http/http_stream_factory_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/socket/ssl_client_socket_pool_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/bidirectional_stream_spdy_impl_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/http2_push_promise_index_test.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_http_stream_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_network_transaction_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_proxy_client_socket_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_session.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_session_fuzzer.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_session_key.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_session_key.h [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_session_pool.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_session_pool_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_session_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/spdy/spdy_stream_unittest.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/websockets/websocket_basic_stream_adapters_test.cc [modify] https://crrev.com/2436b2f13d17a72091143b0041a74d586f584308/net/websockets/websocket_handshake_stream_create_helper_test.cc
,
Dec 11
|
|||
►
Sign in to add a comment |
|||
Comment 1 by mmenke@chromium.org
, Oct 15