TransportSocketParams::on_host_resolution_callback() is fundamentally broken. |
|
Issue descriptionDue to late binding, the TransportSocketParams passed to the socket pool with a request must all be interchangeable. Associating a callback with a TransportSocketParams does not work - the ConnectJob that invokes such a callback may not be the same ConnectJob whose socket is ultimately used to service a socket request. This seems a serious oversight, and means H2's use of callbacks here is pretty broken. Two alternatives: 1) Have HttpStreamFactoryJobs run a DNS lookup in parallel with issuing socket requests. 2) Add a listener API to HostResolvers, so the Jobs can listen for when the host name they care about is resolved. I'm not sure this gets us anything over 1), so it's what I'd suggest.
,
Dec 6
Man! Why is everything so hard :) I agree with your observation about this being broken. Thanks for finding this! I have one question about option #1. I think that in the simple case where there is only 1 pending TCP connection on behalf of an H2 connection, we'd like to make sure that we don't continue the TCP/TLS handshake for the TCP connection, if host resolution allows us to use an existing connection with the same address. With option #1, I can see how the SpdySessionPool would be able to start using the existing H2 connection. But would the SpdySessionPool doing that *also* cause the TCP connection to be aborted? (I'm not familiar enough with the layering here to know)
,
Dec 6
We would continue with SSL negotiation - we currently don't cancel ConnectJobs, even when the socket requests go away. Another option would be to do a DNS lookup first (Possibly after calling a [new] sync method into the socket pool give you an idle socket, if one is already available). That does mean more calls into the socket pool, which may also be a bit expensive. I'll think about this more tomorrow.
,
Dec 10
[rch]: So I think the only thing that really works is this (In HttpStreamFactoryJob): * Check session pool for exact match. * If not found, do full DNS lookup. * If lookup fails, fail. * Check for session using resolved IP. * If not found, start a connection. The DNS result should still be cached (Though technically the cached result could have expired, but even if that happens, it's not the end of the world, I assume?).
,
Dec 10
That sounds quite reasonable. (It almost makes me wonder if we should push the responsibility for host resolution up the stack in general, and just pass down address list to the pool. But that's probably more complicated than it sounds like)
,
Dec 10
Another issue is the proxy case: If we're using an HTTP / SOCKS5 proxy, we should not do the DNS lookup, I assume. |
|
►
Sign in to add a comment |
|
Comment 1 by mmenke@chromium.org
, Dec 6