aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorLoïc Hoguin <[email protected]>2024-01-09 13:06:11 +0100
committerLoïc Hoguin <[email protected]>2024-01-09 13:06:11 +0100
commit906a7ffc3ceaee166f495b071a92ce703f5ce39d (patch)
tree8267573d90b6db772c17d10ac626202227b1e660
parentf0101ffe41b7ec6554c173dc218d60b97ee02d64 (diff)
downloadcowboy-906a7ffc3ceaee166f495b071a92ce703f5ce39d.tar.gz
cowboy-906a7ffc3ceaee166f495b071a92ce703f5ce39d.tar.bz2
cowboy-906a7ffc3ceaee166f495b071a92ce703f5ce39d.zip
Better error message when trying to reply twice
Also crash if trying to push after a reply was sent.
-rw-r--r--src/cowboy_req.erl14
-rw-r--r--test/handlers/resp_h.erl20
-rw-r--r--test/req_SUITE.erl46
3 files changed, 71 insertions, 9 deletions
diff --git a/src/cowboy_req.erl b/src/cowboy_req.erl
index 5d84eed..840d349 100644
--- a/src/cowboy_req.erl
+++ b/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),
diff --git a/test/handlers/resp_h.erl b/test/handlers/resp_h.erl
index 735c654..db3844b 100644
--- a/test/handlers/resp_h.erl
+++ b/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.
diff --git a/test/req_SUITE.erl b/test/req_SUITE.erl
index 92c8092..4bf2739 100644
--- a/test/req_SUITE.erl
+++ b/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."),