search

benoitc / hackney

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

Fix race condition within pool checkout procedure #670

Closed kpribylov closed 3 years ago

kpribylov commented 3 years ago

Hi,

Some time back I made a fix #652 for issue #651 The fix itself is fine, but it seems like we made a hurried decision to add a protective timeout for waiting checkout results https://github.com/benoitc/hackney/pull/652/commits/23d44cbbd42da2f4ddcd49466ece8cbff3285937

The problem is that there are two timers involved in the pool checkout procedure ConnectTimeout and CheckoutTimeout

Therefore we should wait at least ConnectTimeout + CheckoutTimeout to let the intermediary process to complete

However, even doing that we might end up sending unexpected ({checkout,#Ref<...>,{error,checkout_timeout}}) messages to the application process because of having more than one guard timer for the same purpose armed for the same timeout To be absolutely sure we have to ConnectTimeout + CheckoutTimeout + SomeDelta which is opaque

Having said that I suggest rolling back commit https://github.com/benoitc/hackney/pull/652/commits/23d44cbbd42da2f4ddcd49466ece8cbff3285937 and keep managing all the timeouts inside do_checkout

Thanks

kpribylov commented 3 years ago

BTW, looks like that aliases will help us to solve the original problem gracefully https://www.erlang.org/erlang-enhancement-proposals/eep-0053.html

kpribylov commented 3 years ago

I've checked the fix on our load stress setup and it it seems that everything is fine...