Browse Source

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.
Loïc Hoguin 5 years ago
parent
commit
1ba48c58b1
4 changed files with 41 additions and 13 deletions
  1. 9 10
      src/cowboy_http.erl
  2. 3 2
      src/cowboy_http2.erl
  3. 1 1
      test/metrics_SUITE.erl
  4. 28 0
      test/stream_handler_SUITE.erl

+ 9 - 10
src/cowboy_http.erl

@@ -647,13 +647,13 @@ horse_clean_value_ws_end() ->
 	).
 	).
 -endif.
 -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) ->
 		in_state=PS=#ps_header{authority=Authority, version=Version}}, Headers) ->
 	case maps:get(<<"host">>, Headers, undefined) of
 	case maps:get(<<"host">>, Headers, undefined) of
 		undefined when Version =:= 'HTTP/1.1' ->
 		undefined when Version =:= 'HTTP/1.1' ->
 			%% @todo Might want to not close the connection on this and next one.
 			%% @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}},
 			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)'});
 					'HTTP/1.1 requests must include a host header. (RFC7230 5.4)'});
 		undefined ->
 		undefined ->
 			request(Buffer, State, Headers, <<>>, default_port(Transport:secure()));
 			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.
 		%% so we enforce that.
 		_ ->
 		_ ->
 			error_terminate(400, State#state{in_state=PS#ps_header{headers=Headers}},
 			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)'})
 					'The host header is different than the absolute-form authority component. (RFC7230 5.4)'})
 	end.
 	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
 	try cow_http_hd:parse_host(RawHost) of
 		{Host, undefined} ->
 		{Host, undefined} ->
 			request(Buffer, State, Headers, Host, default_port(Transport:secure()));
 			request(Buffer, State, Headers, Host, default_port(Transport:secure()));
 		{Host, Port} when Port > 0, Port =< 65535 ->
 		{Host, Port} when Port > 0, Port =< 65535 ->
 			request(Buffer, State, Headers, Host, Port);
 			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)'})
 				'The port component of the absolute-form is not in the range 0..65535. (RFC7230 2.7.1)'})
 	catch _:_ ->
 	catch _:_ ->
 		error_terminate(400, State#state{in_state=PS#ps_header{headers=Headers}},
 		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)'})
 				'The host header is invalid. (RFC7230 5.4)'})
 	end.
 	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}};
 						true, undefined, fun cow_http_te:stream_chunked/2, {0, 0}};
 				_ ->
 				_ ->
 					error_terminate(400, State0#state{in_state=PS#ps_header{headers=Headers0}},
 					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)'})
 							'Cowboy only supports transfer-encoding: chunked. (RFC7230 3.3.1)'})
 			catch _:_ ->
 			catch _:_ ->
 				error_terminate(400, State0#state{in_state=PS#ps_header{headers=Headers0}},
 				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)'})
 						'The transfer-encoding header is invalid. (RFC7230 3.3.1)'})
 			end;
 			end;
 		#{<<"content-length">> := <<"0">>} ->
 		#{<<"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)
 				cow_http_hd:parse_content_length(BinLength)
 			catch _:_ ->
 			catch _:_ ->
 				error_terminate(400, State0#state{in_state=PS#ps_header{headers=Headers0}},
 				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)'})
 						'The content-length header is invalid. (RFC7230 3.3.2)'})
 			end,
 			end,
 			{Headers0, true, Length, fun cow_http_te:stream_identity/2, {0, Length}};
 			{Headers0, true, Length, fun cow_http_te:stream_identity/2, {0, Length}};

+ 3 - 2
src/cowboy_http2.erl

@@ -497,7 +497,7 @@ headers_frame(State=#state{opts=Opts, streams=Streams}, StreamID, Req) ->
 	end.
 	end.
 
 
 early_error(State0=#state{ref=Ref, opts=Opts, peer=Peer},
 early_error(State0=#state{ref=Ref, opts=Opts, peer=Peer},
-		StreamID, _IsFin, _Headers, #{method := Method},
+		StreamID, _IsFin, Headers, #{method := Method},
 		StatusCode0, HumanReadable) ->
 		StatusCode0, HumanReadable) ->
 	%% We automatically terminate the stream but it is not an error
 	%% We automatically terminate the stream but it is not an error
 	%% per se (at least not in the first implementation).
 	%% per se (at least not in the first implementation).
@@ -508,7 +508,8 @@ early_error(State0=#state{ref=Ref, opts=Opts, peer=Peer},
 	PartialReq = #{
 	PartialReq = #{
 		ref => Ref,
 		ref => Ref,
 		peer => Peer,
 		peer => Peer,
-		method => Method
+		method => Method,
+		headers => headers_to_map(Headers, #{})
 	},
 	},
 	Resp = {response, StatusCode0, RespHeaders0=#{<<"content-length">> => <<"0">>}, <<>>},
 	Resp = {response, StatusCode0, RespHeaders0=#{<<"content-length">> => <<"0">>}, <<>>},
 	try cowboy_stream:early_error(StreamID, Reason, PartialReq, Resp, Opts) of
 	try cowboy_stream:early_error(StreamID, Reason, PartialReq, Resp, Opts) of

+ 1 - 1
test/metrics_SUITE.erl

@@ -314,7 +314,7 @@ do_early_error(Config) ->
 				ref := _,
 				ref := _,
 				pid := From,
 				pid := From,
 				streamid := 1,
 				streamid := 1,
-				reason := {stream_error, 1, protocol_error, _},
+				reason := {stream_error, protocol_error, _},
 				partial_req := #{},
 				partial_req := #{},
 				resp_status := 400,
 				resp_status := 400,
 				resp_headers := ExpectedRespHeaders,
 				resp_headers := ExpectedRespHeaders,

+ 28 - 0
test/stream_handler_SUITE.erl

@@ -248,6 +248,34 @@ do_crash_in_early_error_fatal(Config) ->
 	%% Confirm the connection gets closed.
 	%% Confirm the connection gets closed.
 	gun_down(ConnPid).
 	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) ->
 set_options_ignore_unknown(Config) ->
 	doc("Confirm that unknown options are ignored when using the set_options commands."),
 	doc("Confirm that unknown options are ignored when using the set_options commands."),
 	Self = self(),
 	Self = self(),