Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

IceDragon200
Copy link

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
Copy link
Owner

benoitc commented Sep 14, 2020

@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
Copy link
Author

IceDragon200 commented Sep 15, 2020

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
Copy link

@IceDragon200 testing isolation issues?

@IceDragon200
Copy link
Author

@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
Copy link

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
Copy link
Contributor

SergeTupchiy commented Oct 13, 2020

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 #661

@IceDragon200
Copy link
Author

IceDragon200 commented Oct 15, 2020

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants