New issue
Advanced search Search tips

Issue 912727 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

TransportSocketParams::on_host_resolution_callback() is fundamentally broken.

Project Member Reported by mmenke@chromium.org, Dec 6

Issue description

Due 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.
 
What could happen:

Suppose we have 2 HTTPS requests, both could use H2, and both send requests to the socket pool.  We only have space for one request.  We create a ConnectJob for one request.  The DNS lookup completes!  We tell that one request of the connection.  It tells the ConnectJob to fail...That ConnectJob then tells the other H2 request to fail...That may actually work, if we're racing H2 requests, but I don't think that is really what's intended, or reasonable behavior.

Note that if we have 6 preconnects to the domain, *none* of the host resolution callbacks will be called for any of the requests to a pool, since the socket params passed in by the real requests are never used, just the preconnect ones.  This just seems broken, all around.
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)
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.
[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?).
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)
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