benoitc / hackney

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

Fixed checkout_timeout on errors caused by not canceling the checkout #656

Closed IceDragon200 closed 4 years ago

IceDragon200 commented 4 years ago

Related #643

Hopefully this PR should resolve the the issue with connections being exhausted socket/transport errors, I also did a very, very minuscule bit of code gardening

benoitc commented 4 years ago

@IceDragon200 Thank for the change! However, there is an issue in the CI with your change:


390in function hackney_pool_tests:'-queue_timeout/0-fun-0-'/0 (/home/travis/build/benoitc/hackney/test/hackney_pool_tests.erl, line 43)
391**error:{badmatch,{error,checkout_timeout}}
392  output:<<"">>```

I will have a closer look today anyway
IceDragon200 commented 4 years ago

At a glance it seems the connection is being held over from the previous test (queue_timeout), hackney.close/1 isn't returning the connection it seems

EDIT: managed to get the tests running on my local, so it seems when one or the other runs before the connection isn't being returned

EDIT.2: It's broken on master as well

isaacsanders commented 4 years ago

@IceDragon200 testing isolation issues?

IceDragon200 commented 4 years ago

@isaacsanders It seems like it, and the problem exists in master as well, I haven't had time to revisit the issue due to work, benotic said he would look into it later, but seems he's busy as well

isaacsanders commented 4 years ago

Here's an idea after reading the tests: the queue_timeout test calls ok = hackney:finish_send_body(Ref), and ok = hackney:skip_body(Ref),, which I am led to believe we should call those after the assertion in the queue_timeout test so we return the connection to the pool.

SergeTupchiy commented 4 years ago

This fix has a drawback of not removing request state stored in a process dict (could become a problem for a long-living process) and hackney_manager_refs ETS table:

Erlang/OTP 23 [erts-11.0.3] [source] [64-bit] [smp:4:4] [ds:4:4:10] [async-threads:1] [hipe]

Eshell V11.0.3  (abort with ^G)
1> application:ensure_all_started(hackney).
{ok,[crypto,asn1,public_key,ssl,unicode_util_compat,idna,
     mimerl,certifi,syntax_tools,parse_trans,ssl_verify_fun,
     metrics,hackney]}
2>  F = fun() -> hackney:get("localhost:8000/get") end.
#Fun<erl_eval.45.97283095>
3> Rs=[F() || _ <- lists:seq(1,1000)], io:format("first error: ~p~n",[hd(Rs)]),io:format("last error: ~p~n",[lists:last(Rs)]),f(Rs).
first error: {error,econnrefused}
last error: {error,econnrefused}
ok
4>  hd(get()).
{#Ref<0.3226440105.3247964161.259645>,
 {client,undefined,
         {metrics_ng,metrics_dummy},
         hackney_tcp,"localhost",8000,<<"localhost:8000">>,[],nil,
         nil,#Ref<0.3226440105.3247964161.259645>,true,hackney_pool,
         5000,false,5,false,5,nil,nil,nil,
         {0,{dict,0,16,...}},
         undefined,start,nil,normal,false,...}}
5> length(get()).
1000
12> ets:lookup(hackney_manager_refs, ets:first(hackney_manager_refs)).
[{#Ref<0.3226440105.3248226305.5183>,
  {<0.1132.0>,nil,
   {request_info,default,{1602,621023,669596},"localhost"}}}]
13> 
13> 
13> ets:info(hackney_manager_refs, size).                             
1000

I tried to fix it in https://github.com/benoitc/hackney/pull/661

IceDragon200 commented 4 years ago

661 Solved the issue, so my PR is no longer needed