From a99a8235df642e47cc0f0bcfaaaeb84f634fcda2 Mon Sep 17 00:00:00 2001 From: Leonard Boyce Date: Fri, 17 Aug 2018 12:41:01 -0400 Subject: [PATCH 1/3] (feat) Support 'checkout_timeout' option Allow caller to handle overload when pool is maxed out rather than returning incorrect 'connection_timeout' response --- src/hackney.erl | 2 ++ src/hackney_pool.erl | 6 ++-- test/hackney_pool_checkout_tests.erl | 45 ++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 test/hackney_pool_checkout_tests.erl diff --git a/src/hackney.erl b/src/hackney.erl index 6af74124..e69a1f24 100644 --- a/src/hackney.erl +++ b/src/hackney.erl @@ -228,6 +228,8 @@ request(Method, URL, Headers, Body) -> %%
  • `{proxy, proxy_options()}': to connect via a proxy.
    • %%
  • `insecure': to perform "insecure" SSL connections and %% transfers without checking the certificate
    • +%%
  • `{checkout_timeout, infinity | integer()}': timeout used when +%% checking out a socket from the pool, in milliseconds. Default is 8000
    • %%
  • `{connect_timeout, infinity | integer()}': timeout used when %% establishing a connection, in milliseconds. Default is 8000
    • %%
  • `{recv_timeout, infinity | integer()}': timeout used when diff --git a/src/hackney_pool.erl b/src/hackney_pool.erl index fdabbfb0..ceb97a92 100644 --- a/src/hackney_pool.erl +++ b/src/hackney_pool.erl @@ -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), 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}; @@ -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 diff --git a/test/hackney_pool_checkout_tests.erl b/test/hackney_pool_checkout_tests.erl new file mode 100644 index 00000000..ce6646ee --- /dev/null +++ b/test/hackney_pool_checkout_tests.erl @@ -0,0 +1,45 @@ +-module(hackney_pool_checkout_tests). +-include_lib("eunit/include/eunit.hrl"). +-include("hackney_lib.hrl"). + +%% This seems necessary to list the tests including the generator +dummy_test() -> + ?assertEqual(ok, ok). + +multipart_test_() -> + {setup, fun start/0, fun stop/1, + [checkout_timeout()]}. + +start() -> + error_logger:tty(false), + {ok, _} = application:ensure_all_started(cowboy), + {ok, _} = application:ensure_all_started(hackney), + hackney_pool:start_pool(pool_test, [{pool_size, 1}]), + Host = '_', + Resource = {"/pool", pool_resource, []}, + Dispatch = cowboy_router:compile([{Host, [Resource]}]), + cowboy:start_http(test_server, 10, [{port, 8123}], [{env, [{dispatch, Dispatch}]}]). + +stop({ok, _Pid}) -> + cowboy:stop_listener(test_server), + application:stop(cowboy), + hackney_pool:stop_pool(pool_test), + application:stop(hackney), + error_logger:tty(true), + ok. + +checkout_timeout() -> + fun() -> + URL = <<"http://localhost:8123/pool">>, + Headers = [], + Opts = [{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), + ?assertEqual(Error, checkout_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) + end + end. From 06f7fa8eb5e30e2c67f64cf40ce886b340a514d9 Mon Sep 17 00:00:00 2001 From: Leonard Boyce Date: Fri, 17 Aug 2018 13:35:47 -0400 Subject: [PATCH 2/3] (fix) fix timeout on pool test causing CI failure --- test/hackney_pool_checkout_tests.erl | 45 ---------------------------- test/hackney_pool_tests.erl | 18 +++++++++-- 2 files changed, 16 insertions(+), 47 deletions(-) delete mode 100644 test/hackney_pool_checkout_tests.erl diff --git a/test/hackney_pool_checkout_tests.erl b/test/hackney_pool_checkout_tests.erl deleted file mode 100644 index ce6646ee..00000000 --- a/test/hackney_pool_checkout_tests.erl +++ /dev/null @@ -1,45 +0,0 @@ --module(hackney_pool_checkout_tests). --include_lib("eunit/include/eunit.hrl"). --include("hackney_lib.hrl"). - -%% This seems necessary to list the tests including the generator -dummy_test() -> - ?assertEqual(ok, ok). - -multipart_test_() -> - {setup, fun start/0, fun stop/1, - [checkout_timeout()]}. - -start() -> - error_logger:tty(false), - {ok, _} = application:ensure_all_started(cowboy), - {ok, _} = application:ensure_all_started(hackney), - hackney_pool:start_pool(pool_test, [{pool_size, 1}]), - Host = '_', - Resource = {"/pool", pool_resource, []}, - Dispatch = cowboy_router:compile([{Host, [Resource]}]), - cowboy:start_http(test_server, 10, [{port, 8123}], [{env, [{dispatch, Dispatch}]}]). - -stop({ok, _Pid}) -> - cowboy:stop_listener(test_server), - application:stop(cowboy), - hackney_pool:stop_pool(pool_test), - application:stop(hackney), - error_logger:tty(true), - ok. - -checkout_timeout() -> - fun() -> - URL = <<"http://localhost:8123/pool">>, - Headers = [], - Opts = [{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), - ?assertEqual(Error, checkout_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) - end - end. diff --git a/test/hackney_pool_tests.erl b/test/hackney_pool_tests.erl index 5896835f..4df20a3c 100644 --- a/test/hackney_pool_tests.erl +++ b/test/hackney_pool_tests.erl @@ -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), @@ -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. From 51e6b847d628d494ed1e3121b4a1423a7e9f72eb Mon Sep 17 00:00:00 2001 From: Leonard Boyce Date: Tue, 21 Aug 2018 09:04:53 -0400 Subject: [PATCH 3/3] (fix) #521 trim leading and trailing empty values in parse_qs --- src/hackney_url.erl | 4 ++-- test/hackney_url_tests.erl | 4 +++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/hackney_url.erl b/src/hackney_url.erl index 34e8c733..f40098f0 100644 --- a/src/hackney_url.erl +++ b/src/hackney_url.erl @@ -328,8 +328,8 @@ tohexl(C) when C < 16 -> $a + C - 10. parse_qs(<<>>) -> []; parse_qs(Bin) -> - Tokens = binary:split(Bin, <<"&">>, [trim, global]), - [case binary:split(Token, <<"=">>, [trim]) of + Tokens = binary:split(Bin, <<"&">>, [trim_all, global]), + [case binary:split(Token, <<"=">>, [trim_all]) of [T] -> {urldecode(T), true}; [Name, Value] -> diff --git a/test/hackney_url_tests.erl b/test/hackney_url_tests.erl index 548a94c4..380aaee2 100644 --- a/test/hackney_url_tests.erl +++ b/test/hackney_url_tests.erl @@ -249,7 +249,9 @@ parse_qs_test_() -> Tests = [ {<<"a=b">>, [{<<"a">>,<<"b">>}]}, {<<"a=b&c=d">>, [{<<"a">>,<<"b">>}, {<<"c">>, <<"d">>}]}, - {<<"a=b&c">>, [{<<"a">>,<<"b">>}, {<<"c">>, true}]} + {<<"&a=b&&c=d&">>, [{<<"a">>,<<"b">>}, {<<"c">>, <<"d">>}]}, + {<<"a=b&c">>, [{<<"a">>,<<"b">>}, {<<"c">>, true}]}, + {<<"&a=b&c&&">>, [{<<"a">>,<<"b">>}, {<<"c">>, true}]} ], [{V, fun() -> R = hackney_url:parse_qs(V) end} || {V, R} <- Tests].