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.

Sign up for GitHub

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

Jump to bottom

(feat) Support 'checkout_timeout' option #520

Merged
merged 3 commits into from Sep 10, 2018
Merged

(feat) Support 'checkout_timeout' option #520

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a99a823
(feat) Support 'checkout_timeout' option
leonardb Aug 17, 2018
06f7fa8
(fix) fix timeout on pool test causing CI failure
leonardb Aug 17, 2018
f5dcdc8
(fix) Fall back to connect_timeout when checkout_timeout is not set
leonardb Sep 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/hackney.erl
View file
Open in desktop
Expand Up @@ -228,6 +228,8 @@ request(Method, URL, Headers, Body) ->
%% <li>`{proxy, proxy_options()}': to connect via a proxy.</li>
%% <li>`insecure': to perform "insecure" SSL connections and
%% transfers without checking the certificate</li>
%% <li>`{checkout_timeout, infinity | integer()}': timeout used when
%% checking out a socket from the pool, in milliseconds. Default is 8000</li>
%% <li>`{connect_timeout, infinity | integer()}': timeout used when
%% establishing a connection, in milliseconds. Default is 8000</li>
%% <li>`{recv_timeout, infinity | integer()}': timeout used when
Expand Down
6 changes: 3 additions & 3 deletions src/hackney_pool.erl
View file
Open in desktop
Expand Up @@ -67,9 +67,9 @@ checkout(Host0, Port, Transport, #client{options=Opts}=Client) ->
RequestRef = Client#client.request_ref,
Name = proplists:get_value(pool, Opts, default),
Pool = find_pool(Name, Opts),
ConnectTimeout = proplists:get_value(connect_timeout, Opts, 8000),
CheckoutTimeout = proplists:get_value(checkout_timeout, Opts, 8000),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this introduce a breaking change. I would rather add for the current version add support for both options, that may have a different meaning later or be deprecated. Can you do the change?

Copy link
Contributor Author

@leonardb leonardb Sep 6, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benoitc Sorry for delay, was on vacation. As I read it does support both options with the change. Unless you're wanting to fall back through the options should checkout_timeout not be defined.

IE
checkout_timeout -> connect_timeout -> default 8000

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to fallback for now. ie if connect_timeout is set using it as checkout_timeout.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I was proposing is to fall back to connect_timeout when checkout_timeout is not set.
If we do it the other way around checkout_timeout would not be used when set and the desired separation of connect/checkout would not exist

diff --git a/src/hackney_pool.erl b/src/hackney_pool.erl
index ceb97a9..599fcf5 100644
--- a/src/hackney_pool.erl
+++ b/src/hackney_pool.erl
@@ -67,7 +67,9 @@ checkout(Host0, Port, Transport, #client{options=Opts}=Client) ->
   RequestRef = Client#client.request_ref,
   Name = proplists:get_value(pool, Opts, default),
   Pool = find_pool(Name, Opts),
-  CheckoutTimeout = proplists:get_value(checkout_timeout, Opts, 8000),
+  ConnectTimeout = proplists:get_value(connect_timeout, Opts, 8000),
+  %% Fall back to using connect_timeout if checkout_timeout is not set
+  CheckoutTimeout = proplists:get_value(checkout_timeout, Opts, ConnectTimeout),
   case catch gen_server:call(Pool, {checkout, {Host, Port, Transport},
     Pid, RequestRef}, CheckoutTimeout) of
     {ok, Socket, Owner} ->

case catch gen_server:call(Pool, {checkout, {Host, Port, Transport},
Pid, RequestRef}, ConnectTimeout) of
Pid, RequestRef}, CheckoutTimeout) of
{ok, Socket, Owner} ->
CheckinReference = {Host, Port, Transport},
{ok, {Name, RequestRef, CheckinReference, Owner, Transport}, Socket};
Expand All @@ -82,7 +82,7 @@ checkout(Host0, Port, Transport, #client{options=Opts}=Client) ->
{'EXIT', {timeout, _}} ->
% socket will still checkout so to avoid deadlock we send in a cancellation
gen_server:cast(Pool, {checkout_cancel, {Host, Port, Transport}, RequestRef}),
{error, connect_timeout}
{error, checkout_timeout}
end.

%% @doc release a socket in the pool
Expand Down
18 changes: 16 additions & 2 deletions test/hackney_pool_tests.erl
View file
Open in desktop
Expand Up @@ -8,7 +8,8 @@ dummy_test() ->

multipart_test_() ->
{setup, fun start/0, fun stop/1,
[queue_timeout()]}.
[{timeout, 120, queue_timeout()},
{timeout, 120, checkout_timeout()}]}.

start() ->
error_logger:tty(false),
Expand Down Expand Up @@ -39,7 +40,20 @@ queue_timeout() ->
ok = hackney:finish_send_body(Ref),
{ok, _Status, _Headers, Ref} = hackney:start_response(Ref),
ok = hackney:skip_body(Ref),
{ok, _} = hackney:request(post, URL, Headers, stream, Opts)
{ok, Ref2} = hackney:request(post, URL, Headers, stream, Opts),
hackney:close(Ref2)
end
end.

checkout_timeout() ->
fun() ->
URL = <<"http://localhost:8123/pool">>,
Headers = [],
Opts = [{max_body, 2048}, {pool, pool_test}, {connect_timeout, 1000}, {checkout_timeout, 100}],
case hackney:request(post, URL, Headers, stream, Opts) of
{ok, Ref} ->
{error, Error} = hackney:request(post, URL, Headers, stream, Opts),
hackney:close(Ref),
?assertEqual(Error, checkout_timeout)
end
end.