[3.x] [Net] Remove blocking network address resolution in connect_to_host in websockets #53243
Conversation
jordo
force-pushed
the
PR-wsl-connect_to_host
branch
from
6ab4649 to
7bf08a8
Compare
|
#53262 for master |
|
fixes #53265 for websocket clients |
There was a problem hiding this comment.
I am deeply sorry for the very long review time, I have been sucked up by the work on the replication API.
This change looks okay, it is a bit of a mess that this has to be handled down in every peer separately, but it's definitely an improvement so I would merge this once the review comments are addressed.
Good work 👍
| Vector<String> _protocols; | ||
| Vector<String> _original_protocols; |
There was a problem hiding this comment.
I don't understand why we need the _original_protocols.
It seems there is already the _protocols variable that could be used.
| _connection = _tcp; | ||
|
|
||
| _key = WSLPeer::generate_key(); | ||
| // TODO custom extra headers (allow overriding this too?) |
There was a problem hiding this comment.
We can remove this TODO since we now support extra headers (was a leftover from the other PR I guess).
Faless
changed the title
Faless
dismissed
their stale review
I meant to review the master PR, please disregard this review
akien-mga
force-pushed
the
3.x
branch
2 times, most recently
from
71cb8d3 to
c58391c
Compare
This commit removes blocking address resolution in
connect_to_hostin websockets. This is similar to what happens in HTTPClient.Currently a few other peer's implementations of
connect_to_hoststill include a blocking call toresolve_hostnamewhich unfortunately can block ingetaddrinfofor an unspecified amount of time. This is not good, considering there is a good chance many projects are callingconnect_to_hoston the main engine thread.This PR addresses one of the peers (websocket) with an resolving state, which is similar to what happens in HTTPClient.
I'm hoping eventually we can clean up these connection status state enums into something shared among all the network classes, but until then this and the rest of the network classes
connect_to_hostimplementations should follow what HttpClient does. (Have an intermediate resolving state to avoid blocking inconnect_to_host).As an aside, (as someone who's gone through Apple App review too many times), I'm pretty sure their test environment includes a laggy DNS response env. I have seen app review bring this up a few time with different app submissions, and indeed our project was using the websocket
connect_to_host, and ended up pausing the main thread for much too long waiting for a hostname to resolve. This was easily confirmed with the iOS link conditioner, and setting a manual delay for dns resolution time.