From 1ba48c58b1462fb9d9d8c4d9a43878679aea8eb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Hoguin?= Date: Thu, 3 Oct 2019 15:42:58 +0200 Subject: Make stream_error early_error reasons consistent Now both HTTP/1.1 and HTTP/2 follow the documented format. HTTP/1.1 was including an extra element containing the StreamID before, which was unnecessary because it is also given as argument to the callback. HTTP/2 early_error will now include headers in its PartialReq. --- src/cowboy_http.erl | 19 +++++++++---------- src/cowboy_http2.erl | 5 +++-- test/metrics_SUITE.erl | 2 +- test/stream_handler_SUITE.erl | 28 ++++++++++++++++++++++++++++ 4 files changed, 41 insertions(+), 13 deletions(-) diff --git a/src/cowboy_http.erl b/src/cowboy_http.erl index 505765a..021657a 100644 --- a/src/cowboy_http.erl +++ b/src/cowboy_http.erl @@ -647,13 +647,13 @@ horse_clean_value_ws_end() -> ). -endif. -request(Buffer, State=#state{transport=Transport, in_streamid=StreamID, +request(Buffer, State=#state{transport=Transport, in_state=PS=#ps_header{authority=Authority, version=Version}}, Headers) -> case maps:get(<<"host">>, Headers, undefined) of undefined when Version =:= 'HTTP/1.1' -> %% @todo Might want to not close the connection on this and next one. error_terminate(400, State#state{in_state=PS#ps_header{headers=Headers}}, - {stream_error, StreamID, protocol_error, + {stream_error, protocol_error, 'HTTP/1.1 requests must include a host header. (RFC7230 5.4)'}); undefined -> request(Buffer, State, Headers, <<>>, default_port(Transport:secure())); @@ -667,23 +667,22 @@ request(Buffer, State=#state{transport=Transport, in_streamid=StreamID, %% so we enforce that. _ -> error_terminate(400, State#state{in_state=PS#ps_header{headers=Headers}}, - {stream_error, StreamID, protocol_error, + {stream_error, protocol_error, 'The host header is different than the absolute-form authority component. (RFC7230 5.4)'}) end. -request_parse_host(Buffer, State=#state{transport=Transport, - in_streamid=StreamID, in_state=PS}, Headers, RawHost) -> +request_parse_host(Buffer, State=#state{transport=Transport, in_state=PS}, Headers, RawHost) -> try cow_http_hd:parse_host(RawHost) of {Host, undefined} -> request(Buffer, State, Headers, Host, default_port(Transport:secure())); {Host, Port} when Port > 0, Port =< 65535 -> request(Buffer, State, Headers, Host, Port); _ -> - error_terminate(400, State, {stream_error, StreamID, protocol_error, + error_terminate(400, State, {stream_error, protocol_error, 'The port component of the absolute-form is not in the range 0..65535. (RFC7230 2.7.1)'}) catch _:_ -> error_terminate(400, State#state{in_state=PS#ps_header{headers=Headers}}, - {stream_error, StreamID, protocol_error, + {stream_error, protocol_error, 'The host header is invalid. (RFC7230 5.4)'}) end. @@ -709,11 +708,11 @@ request(Buffer, State0=#state{ref=Ref, transport=Transport, peer=Peer, sock=Sock true, undefined, fun cow_http_te:stream_chunked/2, {0, 0}}; _ -> error_terminate(400, State0#state{in_state=PS#ps_header{headers=Headers0}}, - {stream_error, StreamID, protocol_error, + {stream_error, protocol_error, 'Cowboy only supports transfer-encoding: chunked. (RFC7230 3.3.1)'}) catch _:_ -> error_terminate(400, State0#state{in_state=PS#ps_header{headers=Headers0}}, - {stream_error, StreamID, protocol_error, + {stream_error, protocol_error, 'The transfer-encoding header is invalid. (RFC7230 3.3.1)'}) end; #{<<"content-length">> := <<"0">>} -> @@ -723,7 +722,7 @@ request(Buffer, State0=#state{ref=Ref, transport=Transport, peer=Peer, sock=Sock cow_http_hd:parse_content_length(BinLength) catch _:_ -> error_terminate(400, State0#state{in_state=PS#ps_header{headers=Headers0}}, - {stream_error, StreamID, protocol_error, + {stream_error, protocol_error, 'The content-length header is invalid. (RFC7230 3.3.2)'}) end, {Headers0, true, Length, fun cow_http_te:stream_identity/2, {0, Length}}; diff --git a/src/cowboy_http2.erl b/src/cowboy_http2.erl index 18a367b..f5d80ce 100644 --- a/src/cowboy_http2.erl +++ b/src/cowboy_http2.erl @@ -497,7 +497,7 @@ headers_frame(State=#state{opts=Opts, streams=Streams}, StreamID, Req) -> end. early_error(State0=#state{ref=Ref, opts=Opts, peer=Peer}, - StreamID, _IsFin, _Headers, #{method := Method}, + StreamID, _IsFin, Headers, #{method := Method}, StatusCode0, HumanReadable) -> %% We automatically terminate the stream but it is not an error %% per se (at least not in the first implementation). @@ -508,7 +508,8 @@ early_error(State0=#state{ref=Ref, opts=Opts, peer=Peer}, PartialReq = #{ ref => Ref, peer => Peer, - method => Method + method => Method, + headers => headers_to_map(Headers, #{}) }, Resp = {response, StatusCode0, RespHeaders0=#{<<"content-length">> => <<"0">>}, <<>>}, try cowboy_stream:early_error(StreamID, Reason, PartialReq, Resp, Opts) of diff --git a/test/metrics_SUITE.erl b/test/metrics_SUITE.erl index d9ff7ea..971dd16 100644 --- a/test/metrics_SUITE.erl +++ b/test/metrics_SUITE.erl @@ -314,7 +314,7 @@ do_early_error(Config) -> ref := _, pid := From, streamid := 1, - reason := {stream_error, 1, protocol_error, _}, + reason := {stream_error, protocol_error, _}, partial_req := #{}, resp_status := 400, resp_headers := ExpectedRespHeaders, diff --git a/test/stream_handler_SUITE.erl b/test/stream_handler_SUITE.erl index 02aa437..130447d 100644 --- a/test/stream_handler_SUITE.erl +++ b/test/stream_handler_SUITE.erl @@ -248,6 +248,34 @@ do_crash_in_early_error_fatal(Config) -> %% Confirm the connection gets closed. gun_down(ConnPid). +early_error_stream_error_reason(Config) -> + doc("Confirm that the stream_error given to early_error/5 is consistent between protocols."), + Self = self(), + ConnPid = gun_open(Config), + %% We must use different solutions to hit early_error with a stream_error + %% reason in both protocols. + {Method, Headers, Status, Error} = case config(protocol, Config) of + http -> {<<"GET">>, [{<<"host">>, <<"host:port">>}], 400, protocol_error}; + http2 -> {<<"TRACE">>, [], 501, no_error} + end, + Ref = gun:request(ConnPid, Method, "/long_polling", [ + {<<"accept-encoding">>, <<"gzip">>}, + {<<"x-test-case">>, <<"early_error_stream_error_reason">>}, + {<<"x-test-pid">>, pid_to_list(Self)} + |Headers], <<>>), + %% Confirm init/3 is NOT called. The error occurs before we reach this step. + receive {Self, _, init, _, _, _} -> error(init) after 1000 -> ok end, + %% Confirm terminate/3 is NOT called. We have no state to give to it. + receive {Self, _, terminate, _, _, _} -> error(terminate) after 1000 -> ok end, + %% Confirm early_error/5 is called. + Reason = receive {Self, _, early_error, _, R, _, _, _} -> R after 1000 -> error(timeout) end, + %% Confirm that the Reason is a {stream_error, Reason, Human}. + {stream_error, Error, HumanReadable} = Reason, + true = is_atom(HumanReadable), + %% Receive a 400 or 501 error response. + {response, fin, Status, _} = gun:await(ConnPid, Ref), + ok. + set_options_ignore_unknown(Config) -> doc("Confirm that unknown options are ignored when using the set_options commands."), Self = self(), -- cgit v1.2.3