search

benoitc / hackney

simple HTTP client in Erlang
Other
1.34k stars 427 forks source link

Use destination as pool reference for hackney_http_connect transport #606

Closed ennocramer closed 4 years ago

ennocramer commented 4 years ago

When the HTTP CONNECT method is used for proxied requests, sockets are bound to the destination host and cannot be reused for requests to other hosts. The sockets should therefore be index based on the destination host, not the proxy host.

This setup is still broken when multiple proxies, or proxied and non-proxied connections, are used at the same time, as connections over either proxy are considered identical.

This change should improve the situation for issues #355 and #566

ennocramer commented 4 years ago

I made a mistake in my tests. This PR does not work as it is supposed to. Please do not merge yet.

ennocramer commented 4 years ago

Apologies for the delay with fixing this PR.

I had initially believed it would be easiest to retain the shape of the pool checkin reference and simply swap out the proxy information with the final host. I did not realize that various other functions require access to the proxy transport value and would require passing additional arguments. I now believe the correct fix is to include both the proxy and final host in the checkin reference. This also means that the limitation in my first comment about connections to the same host with different proxy settings no longer applies.

benoitc commented 4 years ago

@ennocramer appologies for the delay to answer, the pr shows the right path anyway. I will take it around. IMO proxies could be handled as an independant resource you link o when making a connection either as a process but lso supervision. I need to think a little bit about it.

benoitc commented 4 years ago

it's now fixed in master. Thanks for your patch it was a good basis !