From 57804f7c86ce90d7fb703d076ca8622d615f2ee5 Mon Sep 17 00:00:00 2001 From: rozap Date: Tue, 9 Jun 2020 15:01:17 -0700 Subject: [PATCH 1/2] Fix erroneous unexpted message * mutually recursive maybe_continue and async_recv functions are really tricky to reason about, but it doesn't seem like it makes sense for maybe_continue to do error handling at all, since async_recv was already doing it * maybe_continue and stream_loop shouldn't select socket messages * we're getting into a state where the socket sends us a message while we're doing other things and we do that receive and then crash * this change makes async_recv the only one to do a select for transport messages * When the server closes the connection and :ssl sends us :ssl_closed, we weren't handling the message, which caused it to leak into the mailbox even after the request was ended * need to flush to make sure it's gone in this case where the server hangs up * caused when Connection: close header is sent fix debugging statement --- src/hackney_response.erl | 12 ++++++++++++ src/hackney_stream.erl | 11 ++--------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/hackney_response.erl b/src/hackney_response.erl index d4f7aa32..284cbdee 100644 --- a/src/hackney_response.erl +++ b/src/hackney_response.erl @@ -349,4 +349,16 @@ close(#client{socket=nil}=Client) -> Client#client{state = closed}; close(#client{transport=Transport, socket=Skt}=Client) -> Transport:close(Skt), + flush(Transport, Skt), Client#client{state = closed, socket=nil}. + + +flush(Transport, Socket) -> + {Msg, MsgClosed, MsgError} = Transport:messages(Socket), + receive + {Msg, Socket, _} -> flush(Transport, Socket) ; + {MsgClosed, Socket} -> flush(Transport, Socket) ; + {MsgError, Socket, _} -> flush(Transport, Socket) + after 0 -> + ok + end. diff --git a/src/hackney_stream.erl b/src/hackney_stream.erl index 7df666dc..cf72e9fd 100644 --- a/src/hackney_stream.erl +++ b/src/hackney_stream.erl @@ -120,11 +120,7 @@ maybe_continue(Parent, Owner, Ref, #client{transport=Transport, exit({owner_down, Owner, Reason}); {system, From, Request} -> sys:handle_system_msg(Request, From, Parent, ?MODULE, [], - {stream_loop, Parent, Owner, Ref, Client}); - Else -> - ?report_trace("stream: unexpected message", [{message, Else}]), - error_logger:error_msg("Unexpected message: ~w~n", [Else]) - + {stream_loop, Parent, Owner, Ref, Client}) after 0 -> stream_loop(Parent, Owner, Ref, Client) end; @@ -146,10 +142,7 @@ maybe_continue(Parent, Owner, Ref, #client{transport=Transport, {system, From, Request} -> sys:handle_system_msg(Request, From, Parent, ?MODULE, [], {maybe_continue, Parent, Owner, Ref, - Client}); - Else -> - ?report_trace("stream: unexpected message", [{message, Else}]), - error_logger:error_msg("Unexpected message: ~w~n", [Else]) + Client}) after 5000 -> Transport:setopts(Socket, [{active, false}]), proc_lib:hibernate(?MODULE, maybe_continue, [Parent, Owner, Ref, From 608ba2ad5dd3148a45338fa029599de0299bed08 Mon Sep 17 00:00:00 2001 From: rozap Date: Thu, 18 Jun 2020 09:46:04 -0700 Subject: [PATCH 2/2] Handle empty trailers case * bug i think appeared in 6ddc33c1 * when streaming the response, we terminate the parse recursive calls early since there is no match for {:more, {hparser, ...:on_trailers...}} case --- src/hackney_response.erl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/hackney_response.erl b/src/hackney_response.erl index 284cbdee..a1277ed2 100644 --- a/src/hackney_response.erl +++ b/src/hackney_response.erl @@ -147,6 +147,8 @@ stream_body(Client=#client{parser=Parser, clen=CLen, te=TE}) -> stream_body(Data, #client{parser=Parser}=Client) -> stream_body1(hackney_http:execute(Parser, Data), Client). +stream_body1({more, Parser}, Client) -> + stream_body_recv(<<>>, Client#client{parser=Parser}); stream_body1({more, Parser, Buffer}, Client) -> stream_body_recv(Buffer, Client#client{parser=Parser}); stream_body1({ok, Data, Parser}, Client) ->