Better error message when trying to reply twice

Also crash if trying to push after a reply was sent.

src/cowboy_req.erl

@@ -714,7 +714,7 @@ set_resp_cookie(Name, Value, Req, Opts) ->
 	RespCookies = maps:get(resp_cookies, Req, #{}),
 	Req#{resp_cookies => RespCookies#{Name => Cookie}}.
 
-%% @todo We could add has_resp_cookie and delete_resp_cookie now.
+%% @todo We could add has_resp_cookie and unset_resp_cookie now.
 
 -spec set_resp_header(binary(), iodata(), Req)
 	-> Req when Req::req().
@@ -779,7 +779,8 @@ inform(Status, Req) ->
 
 -spec inform(cowboy:http_status(), cowboy:http_headers(), req()) -> ok.
 inform(_, _, #{has_sent_resp := _}) ->
-	error(function_clause); %% @todo Better error message.
+	exit({response_error, response_already_sent,
+		'The final response has already been sent.'});
 inform(Status, Headers, Req) when is_integer(Status); is_binary(Status) ->
 	cast({inform, Status, Headers}, Req).
 
@@ -797,7 +798,8 @@ reply(Status, Headers, Req) ->
 -spec reply(cowboy:http_status(), cowboy:http_headers(), resp_body(), Req)
 	-> Req when Req::req().
 reply(_, _, _, #{has_sent_resp := _}) ->
-	error(function_clause); %% @todo Better error message.
+	exit({response_error, response_already_sent,
+		'The final response has already been sent.'});
 reply(Status, Headers, {sendfile, _, 0, _}, Req)
 		when is_integer(Status); is_binary(Status) ->
 	do_reply(Status, Headers#{
@@ -853,7 +855,8 @@ stream_reply(Status, Req) ->
 -spec stream_reply(cowboy:http_status(), cowboy:http_headers(), Req)
 	-> Req when Req::req().
 stream_reply(_, _, #{has_sent_resp := _}) ->
-	error(function_clause);
+	exit({response_error, response_already_sent,
+		'The final response has already been sent.'});
 %% 204 and 304 responses must NOT send a body. We therefore
 %% transform the call to a full response and expect the user
 %% to NOT call stream_body/3 afterwards. (RFC7230 3.3)
@@ -916,6 +919,9 @@ push(Path, Headers, Req) ->
 %% @todo Path, Headers, Opts, everything should be in proper binary,
 %% or normalized when creating the Req object.
 -spec push(iodata(), cowboy:http_headers(), req(), push_opts()) -> ok.
+push(_, _, #{has_sent_resp := _}, _) ->
+	exit({response_error, response_already_sent,
+		'The final response has already been sent.'});
 push(Path, Headers, Req=#{scheme := Scheme0, host := Host0, port := Port0}, Opts) ->
 	Method = maps:get(method, Opts, <<"GET">>),
 	Scheme = maps:get(scheme, Opts, Scheme0),

test/handlers/resp_h.erl

@@ -130,6 +130,10 @@ do(<<"inform2">>, Req0, Opts) ->
 		<<"twice">> ->
 			cowboy_req:inform(102, Req0),
 			cowboy_req:inform(102, Req0);
+		<<"after_reply">> ->
+			ct_helper:ignore(cowboy_req, inform, 3),
+			Req1 = cowboy_req:reply(200, Req0),
+			cowboy_req:inform(102, Req1);
 		Status ->
 			cowboy_req:inform(binary_to_integer(Status), Req0)
 	end,
@@ -146,6 +150,10 @@ do(<<"inform3">>, Req0, Opts) ->
 		<<"twice">> ->
 			cowboy_req:inform(102, Headers, Req0),
 			cowboy_req:inform(102, Headers, Req0);
+		<<"after_reply">> ->
+			ct_helper:ignore(cowboy_req, inform, 3),
+			Req1 = cowboy_req:reply(200, Req0),
+			cowboy_req:inform(102, Headers, Req1);
 		Status ->
 			cowboy_req:inform(binary_to_integer(Status), Headers, Req0)
 	end,
@@ -215,6 +223,13 @@ do(<<"stream_reply2">>, Req0, Opts) ->
 			Req = cowboy_req:stream_reply(304, Req0),
 			stream_body(Req),
 			{ok, Req, Opts};
+		<<"twice">> ->
+			ct_helper:ignore(cowboy_req, stream_reply, 3),
+			Req1 = cowboy_req:stream_reply(200, Req0),
+			%% We will crash here so the body shouldn't be sent.
+			Req = cowboy_req:stream_reply(200, Req1),
+			stream_body(Req),
+			{ok, Req, Opts};
 		Status ->
 			Req = cowboy_req:stream_reply(binary_to_integer(Status), Req0),
 			stream_body(Req),
@@ -403,6 +418,11 @@ do(<<"push">>, Req, Opts) ->
 		<<"qs">> ->
 			cowboy_req:push("/static/style.css", #{<<"accept">> => <<"text/css">>}, Req,
 				#{qs => <<"server=cowboy&version=2.0">>});
+		<<"after_reply">> ->
+			ct_helper:ignore(cowboy_req, push, 4),
+			Req1 = cowboy_req:reply(200, Req),
+			%% We will crash here so no need to worry about propagating Req1.
+			cowboy_req:push("/static/style.css", #{<<"accept">> => <<"text/css">>}, Req1);
 		_ ->
 			cowboy_req:push("/static/style.css", #{<<"accept">> => <<"text/css">>}, Req),
 			%% The text/plain mime is not defined by default, so a 406 will be returned.

test/req_SUITE.erl

@@ -883,6 +883,8 @@ inform2(Config) ->
 	{102, [], 200, _, _} = do_get_inform("/resp/inform2/binary", Config),
 	{500, _} = do_get_inform("/resp/inform2/error", Config),
 	{102, [], 200, _, _} = do_get_inform("/resp/inform2/twice", Config),
+	%% @todo How to test this properly? This isn't enough.
+	{200, _} = do_get_inform("/resp/inform2/after_reply", Config),
 	ok.
 
 inform3(Config) ->
@@ -892,6 +894,8 @@ inform3(Config) ->
 	{102, Headers, 200, _, _} = do_get_inform("/resp/inform3/binary", Config),
 	{500, _} = do_get_inform("/resp/inform3/error", Config),
 	{102, Headers, 200, _, _} = do_get_inform("/resp/inform3/twice", Config),
+	%% @todo How to test this properly? This isn't enough.
+	{200, _} = do_get_inform("/resp/inform3/after_reply", Config),
 	ok.
 
 reply2(Config) ->
@@ -901,8 +905,7 @@ reply2(Config) ->
 	{404, _, _} = do_get("/resp/reply2/404", Config),
 	{200, _, _} = do_get("/resp/reply2/binary", Config),
 	{500, _, _} = do_get("/resp/reply2/error", Config),
-	%% @todo We want to crash when reply or stream_reply is called twice.
-	%% How to test this properly? This isn't enough.
+	%% @todo How to test this properly? This isn't enough.
 	{200, _, _} = do_get("/resp/reply2/twice", Config),
 	ok.
 
@@ -925,8 +928,6 @@ reply4(Config) ->
 	{500, _, _} = do_get("/resp/reply4/error", Config),
 	ok.
 
-%% @todo Crash when stream_reply is called twice.
-
 stream_reply2(Config) ->
 	doc("Response with default headers and streamed body."),
 	Body = <<0:8000000>>,
@@ -937,6 +938,33 @@ stream_reply2(Config) ->
 	{500, _, _} = do_get("/resp/stream_reply2/error", Config),
 	ok.
 
+stream_reply2_twice(Config) ->
+	doc("Attempting to stream a response twice results in a crash. "
+		"This crash can only be properly detected in HTTP/2."),
+	ConnPid = gun_open(Config),
+	Ref = gun:get(ConnPid, "/resp/stream_reply2/twice",
+		[{<<"accept-encoding">>, <<"gzip">>}]),
+	{response, nofin, 200, _} = gun:await(ConnPid, Ref, infinity),
+	Protocol = config(protocol, Config),
+	Flavor = config(flavor, Config),
+	case {Protocol, Flavor, gun:await_body(ConnPid, Ref, infinity)} of
+		%% In HTTP/1.1 we cannot propagate an error at that point.
+		%% The response will simply not have a body.
+		{http, vanilla, {ok, <<>>}} ->
+			ok;
+		%% When compression was used we do get gzip headers. But
+		%% we do not have any data in the zlib stream.
+		{http, compress, {ok, Data}} ->
+			Z = zlib:open(),
+			zlib:inflateInit(Z, 31),
+			0 = iolist_size(zlib:inflate(Z, Data)),
+			ok;
+		%% In HTTP/2 the stream gets reset with an appropriate error.
+		{http2, _, {error, {stream_error, {stream_error, internal_error, _}}}} ->
+			ok
+	end,
+	gun:close(ConnPid).
+
 stream_reply3(Config) ->
 	doc("Response with additional headers and streamed body."),
 	Body = <<0:8000000>>,
@@ -1152,6 +1180,14 @@ push(Config) ->
 		http2 -> do_push_http2(Config)
 	end.
 
+push_after_reply(Config) ->
+	doc("Trying to push a response after the final response results in a crash."),
+	ConnPid = gun_open(Config),
+	Ref = gun:get(ConnPid, "/resp/push/after_reply", []),
+	%% @todo How to test this properly? This isn't enough.
+	{response, fin, 200, _} = gun:await(ConnPid, Ref, infinity),
+	gun:close(ConnPid).
+
 push_method(Config) ->
 	case config(protocol, Config) of
 		http -> do_push_http("/resp/push/method", Config);
@@ -1176,7 +1212,7 @@ do_push_http(Path, Config) ->
 	ConnPid = gun_open(Config),
 	Ref = gun:get(ConnPid, Path, []),
 	{response, fin, 200, _} = gun:await(ConnPid, Ref, infinity),
-	ok.
+	gun:close(ConnPid).
 
 do_push_http2(Config) ->
 	doc("Pushed responses."),