From b9a2da76f0a4d84b749c4ac6ef8d269994385593 Mon Sep 17 00:00:00 2001 From: Luca Favatella Date: Mon, 19 Mar 2018 10:36:18 +0000 Subject: inets: Delete obsolete comment in httpc_response:result --- lib/inets/src/http_client/httpc_response.erl | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/inets/src/http_client/httpc_response.erl b/lib/inets/src/http_client/httpc_response.erl index 58ab9144df..92dc9b0e02 100644 --- a/lib/inets/src/http_client/httpc_response.erl +++ b/lib/inets/src/http_client/httpc_response.erl @@ -83,7 +83,6 @@ whole_body(Body, Length) -> %% result(Response, Request) -> %% Response - {StatusLine, Headers, Body} %% Request - #request{} -%% Session - #tcp_session{} %% %% Description: Checks the status code ... %%------------------------------------------------------------------------- -- cgit v1.2.3 From 2591eddc8db45c89e01ceefa009537aaf660ac82 Mon Sep 17 00:00:00 2001 From: Luca Favatella Date: Fri, 16 Mar 2018 21:03:19 +0000 Subject: inets: Remove httpc dead code re init error ... i.e. references to `connect_failed` and `send_failed`, unused since 5d32eaf750 . --- lib/inets/doc/src/httpc.xml | 3 +-- lib/inets/src/http_client/httpc_handler.erl | 21 +-------------------- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/lib/inets/doc/src/httpc.xml b/lib/inets/doc/src/httpc.xml index 14662f257c..ffc6fec518 100644 --- a/lib/inets/doc/src/httpc.xml +++ b/lib/inets/doc/src/httpc.xml @@ -312,8 +312,7 @@ Body = string() | binary() Profile = profile() | pid() When started stand_alone only the pid can be used. - Reason = {connect_failed, term()} | - {send_failed, term()} | term() + Reason = term() diff --git a/lib/inets/src/http_client/httpc_handler.erl b/lib/inets/src/http_client/httpc_handler.erl index 9b09832eb8..327aec4baa 100644 --- a/lib/inets/src/http_client/httpc_handler.erl +++ b/lib/inets/src/http_client/httpc_handler.erl @@ -48,12 +48,10 @@ queue_timer :: reference() | 'undefined' }). --type session_failed() :: {'connect_failed',term()} | {'send_failed',term()}. - -record(state, { request :: request() | 'undefined', - session :: session() | session_failed() | 'undefined', + session :: session() | 'undefined', status_line, % {Version, StatusCode, ReasonPharse} headers :: http_response_h() | 'undefined', body :: binary() | 'undefined', @@ -295,23 +293,6 @@ handle_info(Info, State) -> %% Description: Shutdown the httpc_handler %%-------------------------------------------------------------------- -%% Init error there is no socket to be closed. -terminate(normal, - #state{request = Request, - session = {send_failed, _} = Reason} = State) -> - maybe_send_answer(Request, - httpc_response:error(Request, Reason), - State), - ok; - -terminate(normal, - #state{request = Request, - session = {connect_failed, _} = Reason} = State) -> - maybe_send_answer(Request, - httpc_response:error(Request, Reason), - State), - ok; - terminate(normal, #state{session = undefined}) -> ok; -- cgit v1.2.3 From 4868c38d1653ee9389ec024214509661818baf2a Mon Sep 17 00:00:00 2001 From: Luca Favatella Date: Fri, 16 Mar 2018 21:26:53 +0000 Subject: inets: Enable stronger Dialyzer checks in httpc_handler --- lib/inets/src/http_client/httpc_handler.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/inets/src/http_client/httpc_handler.erl b/lib/inets/src/http_client/httpc_handler.erl index 327aec4baa..8311d1ed76 100644 --- a/lib/inets/src/http_client/httpc_handler.erl +++ b/lib/inets/src/http_client/httpc_handler.erl @@ -58,7 +58,7 @@ mfa, % {Module, Function, Args} pipeline = queue:new() :: queue:queue(), keep_alive = queue:new() :: queue:queue(), - status, % undefined | new | pipeline | keep_alive | close | {ssl_tunnel, Request} + status :: undefined | new | pipeline | keep_alive | close | {ssl_tunnel, request()}, canceled = [], % [RequestId] max_header_size = nolimit :: nolimit | integer(), max_body_size = nolimit :: nolimit | integer(), -- cgit v1.2.3 From c061eb5f8b6b0fdf3468945d8f6eea818ac4f478 Mon Sep 17 00:00:00 2001 From: Luca Favatella Date: Mon, 19 Mar 2018 19:16:29 +0000 Subject: inets: Make httpc error reason more informative for unexpected errors --- lib/inets/src/http_client/httpc_handler.erl | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/inets/src/http_client/httpc_handler.erl b/lib/inets/src/http_client/httpc_handler.erl index 8311d1ed76..0ca8155054 100644 --- a/lib/inets/src/http_client/httpc_handler.erl +++ b/lib/inets/src/http_client/httpc_handler.erl @@ -253,8 +253,9 @@ handle_call(Request, From, State) -> Result -> Result catch - _:Reason -> - {stop, {shutdown, Reason} , State} + Class:Reason -> + ST = erlang:get_stacktrace(), + {stop, {shutdown, {{Class, Reason}, ST}}, State} end. @@ -269,8 +270,9 @@ handle_cast(Msg, State) -> Result -> Result catch - _:Reason -> - {stop, {shutdown, Reason} , State} + Class:Reason -> + ST = erlang:get_stacktrace(), + {stop, {shutdown, {{Class, Reason}, ST}}, State} end. %%-------------------------------------------------------------------- @@ -284,8 +286,9 @@ handle_info(Info, State) -> Result -> Result catch - _:Reason -> - {stop, {shutdown, Reason} , State} + Class:Reason -> + ST = erlang:get_stacktrace(), + {stop, {shutdown, {{Class, Reason}, ST}}, State} end. %%-------------------------------------------------------------------- @@ -569,11 +572,12 @@ do_handle_info({Proto, _Socket, Data}, activate_once(Session), {noreply, State#state{mfa = NewMFA}} catch - _:Reason -> + Class:Reason -> + ST = erlang:get_stacktrace(), ClientReason = {could_not_parse_as_http, Data}, ClientErrMsg = httpc_response:error(Request, ClientReason), NewState = answer_request(Request, ClientErrMsg, State), - {stop, {shutdown, Reason}, NewState} + {stop, {shutdown, {{Class, Reason}, ST}}, NewState} end; do_handle_info({Proto, Socket, Data}, -- cgit v1.2.3 From cef626165bc62edcc78c3be0c2cb107ae961b805 Mon Sep 17 00:00:00 2001 From: Luca Favatella Date: Fri, 16 Mar 2018 13:01:43 +0000 Subject: inets: Fix profile used in persistent_connection tests in httpc_SUITE It looks like a typo, though I did not experience impact of this on tests. --- lib/inets/test/httpc_SUITE.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/inets/test/httpc_SUITE.erl b/lib/inets/test/httpc_SUITE.erl index 38705372c9..c8eaa4b3df 100644 --- a/lib/inets/test/httpc_SUITE.erl +++ b/lib/inets/test/httpc_SUITE.erl @@ -233,7 +233,7 @@ init_per_testcase(pipeline, Config) -> init_per_testcase(persistent_connection, Config) -> inets:start(httpc, [{profile, persistent}]), httpc:set_options([{keep_alive_timeout, 50000}, - {max_keep_alive_length, 3}], persistent_connection), + {max_keep_alive_length, 3}], persistent), Config; init_per_testcase(wait_for_whole_response, Config) -> -- cgit v1.2.3 From 9a19dc099198585bd4f019116663d5b8be506ce0 Mon Sep 17 00:00:00 2001 From: Luca Favatella Date: Fri, 16 Mar 2018 18:00:00 +0000 Subject: inets: Teach httpc to honour `Connection: close` from server From https://tools.ietf.org/html/rfc7230#section-6.6 > A client that receives a "close" connection option MUST cease sending requests on that connection and close the connection after reading the response message containing the "close"; if additional pipelined requests had been sent on the connection, the client SHOULD NOT assume that they will be processed by the server. Notes on tests: * The new tests are added only in group sim_http and not sim_https because the same test approach appears to be not valid because when processing the first response the server already sent data (> 0) for the TLS/SSL handshake; * The order of tests is relevant as it appears some test cases leave reusable sessions behind. --- lib/inets/src/http_client/httpc_handler.erl | 32 +++++++--- lib/inets/test/httpc_SUITE.erl | 95 ++++++++++++++++++++++++++++- 2 files changed, 117 insertions(+), 10 deletions(-) diff --git a/lib/inets/src/http_client/httpc_handler.erl b/lib/inets/src/http_client/httpc_handler.erl index 0ca8155054..ca29523f84 100644 --- a/lib/inets/src/http_client/httpc_handler.erl +++ b/lib/inets/src/http_client/httpc_handler.erl @@ -1043,15 +1043,29 @@ handle_response(#state{status = new} = State) -> ?hcrd("handle response - status = new", []), handle_response(try_to_enable_pipeline_or_keep_alive(State)); -handle_response(#state{request = Request, - status = Status, - session = Session, - status_line = StatusLine, - headers = Headers, - body = Body, - options = Options, - profile_name = ProfileName} = State) - when Status =/= new -> +handle_response(#state{status = Status0} = State0) when Status0 =/= new -> + Status = + case Status0 of + close -> close; + _ -> + case State0#state.headers of + undefined -> Status0; + _ -> + case httpc_response:is_server_closing( + State0#state.headers) of + true -> close; + false -> Status0 + end + end + end, + State = State0#state{status = Status}, + #state{request = Request, + session = Session, + status_line = StatusLine, + headers = Headers, + body = Body, + options = Options, + profile_name = ProfileName} = State, handle_cookies(Headers, Request, Options, ProfileName), case httpc_response:result({StatusLine, Headers, Body}, Request) of %% 100-continue diff --git a/lib/inets/test/httpc_SUITE.erl b/lib/inets/test/httpc_SUITE.erl index c8eaa4b3df..287db81bad 100644 --- a/lib/inets/test/httpc_SUITE.erl +++ b/lib/inets/test/httpc_SUITE.erl @@ -67,7 +67,7 @@ groups() -> %% process_leak_on_keepalive is depending on stream_fun_server_close %% and it shall be the last test case in the suite otherwise cookie %% will fail. - {sim_http, [], only_simulated() ++ [process_leak_on_keepalive]}, + {sim_http, [], only_simulated() ++ server_closing_connection() ++ [process_leak_on_keepalive]}, {http_internal, [], real_requests_esi()}, {http_unix_socket, [], simulated_unix_socket()}, {https, [], real_requests()}, @@ -154,6 +154,12 @@ only_simulated() -> stream_fun_server_close ]. +server_closing_connection() -> + [ + server_closing_connection_on_first_response, + server_closing_connection_on_second_response + ]. + misc() -> [ server_does_not_exist, @@ -252,6 +258,24 @@ end_per_testcase(pipeline, _Config) -> inets:stop(httpc, pipeline); end_per_testcase(persistent_connection, _Config) -> inets:stop(httpc, persistent); +end_per_testcase(Case, Config) + when Case == server_closing_connection_on_first_response; + Case == server_closing_connection_on_second_response -> + %% Test case uses at most one session. Ensure no leftover + %% sessions left behind. + {_, Status} = proplists:lookup(tc_status, Config), + ShallCleanup = case Status of + ok -> true; + {failed, _} -> true; + {skipped, _} -> false + end, + if ShallCleanup -> + httpc:request(url(group_name(Config), "/just_close.html", Config)), + ok; + true -> + ct:pal("Not cleaning up because test case status was ~p", [Status]), + ok + end; end_per_testcase(_Case, _Config) -> ok. @@ -1274,6 +1298,53 @@ stream_fun_server_close(Config) when is_list(Config) -> ct:fail(did_not_receive_close) end. +%%-------------------------------------------------------------------- +server_closing_connection_on_first_response() -> + [{doc, "Client receives \"Connection: close\" on first response." + "A client that receives a \"close\" connection option MUST cease sending" + "requests on that connection and close the connection after reading" + "the response message containing the \"close\""}]. +server_closing_connection_on_first_response(Config) when is_list(Config) -> + ReqSrvSendOctFun = + fun(V, U, S) -> + {ok, {{V, S, _}, Headers0, []}} = + httpc:request(get, {U, []}, [{version, V}], []), + {_, SendOctStr} = + proplists:lookup("x-socket-stat-send-oct", Headers0), + list_to_integer(SendOctStr) + end, + V = "HTTP/1.1", + Url0 = url(group_name(Config), "/http_1_1_send_oct.html", Config), + Url1 = url(group_name(Config), "/http_1_1_send_oct_and_connection_close.html", Config), + %% Test case assumes at most one reusable past session. + _ = ReqSrvSendOctFun(V, Url1, 204), + 0 = ReqSrvSendOctFun(V, Url0, 204), + ok. + +%%-------------------------------------------------------------------- +server_closing_connection_on_second_response() -> + [{doc, "Client receives \"Connection: close\" on second response." + "A client that receives a \"close\" connection option MUST cease sending" + "requests on that connection and close the connection after reading" + "the response message containing the \"close\""}]. +server_closing_connection_on_second_response(Config) when is_list(Config) -> + ReqSrvSendOctFun = + fun(V, U, S) -> + {ok, {{V, S, _}, Headers0, []}} = + httpc:request(get, {U, []}, [{version, V}], []), + {_, SendOctStr} = + proplists:lookup("x-socket-stat-send-oct", Headers0), + list_to_integer(SendOctStr) + end, + V = "HTTP/1.1", + Url0 = url(group_name(Config), "/http_1_1_send_oct.html", Config), + Url1 = url(group_name(Config), "/http_1_1_send_oct_and_connection_close.html", Config), + %% Test case assumes no reusable past sessions. + SendOct0 = 0 = ReqSrvSendOctFun(V, Url0, 204), + case ReqSrvSendOctFun(V, Url1, 204) of SendOct1 when SendOct1 > SendOct0 -> ok end, + 0 = ReqSrvSendOctFun(V, Url0, 204), + ok. + %%-------------------------------------------------------------------- slow_connection() -> [{doc, "Test that a request on a slow keep-alive connection won't crash the httpc_manager"}]. @@ -2232,10 +2303,32 @@ handle_uri("GET","/v1/kv/foo",_,_,_,_) -> "Content-Length: 24\r\n" ++ "Content-Type: application/json\r\n\r\n" ++ "[{\"Value\": \"aGVsbG8=\"}]\n"; +handle_uri(_,"/http_1_1_send_oct.html",_,_,Socket,_) -> + "HTTP/1.1 204 No Content\r\n" ++ + "X-Socket-Stat-Send-Oct: " ++ integer_to_list(get_stat(Socket, send_oct)) ++ "\r\n" ++ + "\r\n"; +handle_uri(_,"/http_1_1_send_oct_and_connection_close.html",_,_,Socket,_) -> + "HTTP/1.1 204 No Content\r\n" ++ + "X-Socket-Stat-Send-Oct: " ++ integer_to_list(get_stat(Socket, send_oct)) ++ "\r\n" ++ + "Connection: close\r\n" ++ + "\r\n"; handle_uri(_,_,_,_,_,DefaultResponse) -> DefaultResponse. +get_stat(S, Opt) -> + case getstat(S, [Opt]) of + {ok, [{Opt, V}]} when is_integer(V) -> + V; + {error, _} = E -> + E + end. + +getstat(#sslsocket{} = S, Opts) -> + ssl:getstat(S, Opts); +getstat(S, Opts) -> + inet:getstat(S, Opts). + url_start(#sslsocket{}) -> {ok,Host} = inet:gethostname(), ?TLS_URL_START ++ Host ++ ":"; -- cgit v1.2.3 From 2f7731260c4624cd5a324cb7c6ea9c124cf9ea4a Mon Sep 17 00:00:00 2001 From: Luca Favatella Date: Thu, 5 Apr 2018 15:02:43 +0100 Subject: inets: Avoid `erlang:get_stacktrace/0` deprecated in OTP 21 --- lib/inets/src/http_client/httpc_handler.erl | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lib/inets/src/http_client/httpc_handler.erl b/lib/inets/src/http_client/httpc_handler.erl index ca29523f84..945b1b94b6 100644 --- a/lib/inets/src/http_client/httpc_handler.erl +++ b/lib/inets/src/http_client/httpc_handler.erl @@ -253,8 +253,7 @@ handle_call(Request, From, State) -> Result -> Result catch - Class:Reason -> - ST = erlang:get_stacktrace(), + Class:Reason:ST -> {stop, {shutdown, {{Class, Reason}, ST}}, State} end. @@ -270,8 +269,7 @@ handle_cast(Msg, State) -> Result -> Result catch - Class:Reason -> - ST = erlang:get_stacktrace(), + Class:Reason:ST -> {stop, {shutdown, {{Class, Reason}, ST}}, State} end. @@ -286,8 +284,7 @@ handle_info(Info, State) -> Result -> Result catch - Class:Reason -> - ST = erlang:get_stacktrace(), + Class:Reason:ST -> {stop, {shutdown, {{Class, Reason}, ST}}, State} end. @@ -572,8 +569,7 @@ do_handle_info({Proto, _Socket, Data}, activate_once(Session), {noreply, State#state{mfa = NewMFA}} catch - Class:Reason -> - ST = erlang:get_stacktrace(), + Class:Reason:ST -> ClientReason = {could_not_parse_as_http, Data}, ClientErrMsg = httpc_response:error(Request, ClientReason), NewState = answer_request(Request, ClientErrMsg, State), -- cgit v1.2.3 From b1567acdbe2c0189e39eb5f0cb696773dd7d1961 Mon Sep 17 00:00:00 2001 From: Luca Favatella Date: Fri, 6 Apr 2018 10:24:29 +0100 Subject: inets: Work around warning in test ... addressing PR comment https://github.com/erlang/otp/pull/1752#pullrequestreview-107945563 --- lib/inets/test/httpc_SUITE.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/inets/test/httpc_SUITE.erl b/lib/inets/test/httpc_SUITE.erl index 287db81bad..1116fdb1b6 100644 --- a/lib/inets/test/httpc_SUITE.erl +++ b/lib/inets/test/httpc_SUITE.erl @@ -269,7 +269,7 @@ end_per_testcase(Case, Config) {failed, _} -> true; {skipped, _} -> false end, - if ShallCleanup -> + if ShallCleanup =:= true -> httpc:request(url(group_name(Config), "/just_close.html", Config)), ok; true -> -- cgit v1.2.3 From d9d40dd446d7e7783ceaff62f0bc5f74e556d119 Mon Sep 17 00:00:00 2001 From: Luca Favatella Date: Fri, 6 Apr 2018 10:45:28 +0100 Subject: inets: Improve readability of handling of server `Connection: close` Addresses https://github.com/erlang/otp/pull/1752#discussion_r177970060 --- lib/inets/src/http_client/httpc_handler.erl | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/lib/inets/src/http_client/httpc_handler.erl b/lib/inets/src/http_client/httpc_handler.erl index 945b1b94b6..eeb08ce0ee 100644 --- a/lib/inets/src/http_client/httpc_handler.erl +++ b/lib/inets/src/http_client/httpc_handler.erl @@ -1040,21 +1040,7 @@ handle_response(#state{status = new} = State) -> handle_response(try_to_enable_pipeline_or_keep_alive(State)); handle_response(#state{status = Status0} = State0) when Status0 =/= new -> - Status = - case Status0 of - close -> close; - _ -> - case State0#state.headers of - undefined -> Status0; - _ -> - case httpc_response:is_server_closing( - State0#state.headers) of - true -> close; - false -> Status0 - end - end - end, - State = State0#state{status = Status}, + State = handle_server_closing(State0), #state{request = Request, session = Session, status_line = StatusLine, @@ -1325,6 +1311,14 @@ try_to_enable_pipeline_or_keep_alive( State#state{status = close} end. +handle_server_closing(State = #state{status = close}) -> State; +handle_server_closing(State = #state{headers = undefined}) -> State; +handle_server_closing(State = #state{headers = Headers}) -> + case httpc_response:is_server_closing(Headers) of + true -> State#state{status = close}; + false -> State + end. + answer_request(#request{id = RequestId, from = From} = Request, Msg, #state{session = Session, timers = Timers, -- cgit v1.2.3