Do not send a 101 after a final response in switch_protocol

doc/src/guide/migrating_from_2.4.asciidoc

@@ -81,6 +81,11 @@ also been worked on.
 
 * Improve the validation of HTTP/1.1 absolute-form requests.
 
+* When the `switch_protocol` is used after a response was
+  sent, Cowboy will no longer attempt to send the 101 informational
+  response for the protocol upgrade. This caused a crash of the
+  connection previously.
+
 * Errors that occur when a callback returned by
   `content_types_provided` does not exist have been improved.
 

doc/src/manual/cowboy_stream.asciidoc

@@ -205,6 +205,9 @@ Contains the headers that will be sent in the 101 response,
 along with the module implementing the protocol we are
 switching to and its initial state.
 
+Note that the 101 informational response will not be sent
+after a final response.
+
 === stop
 
 Stop the stream.

src/cowboy_http.erl

@@ -1064,7 +1064,7 @@ commands(State0=#state{socket=Socket, transport=Transport}, StreamID,
 	end;
 %% Protocol takeover.
 commands(State0=#state{ref=Ref, parent=Parent, socket=Socket, transport=Transport,
-		opts=Opts, children=Children}, StreamID,
+		out_state=OutState, opts=Opts, children=Children}, StreamID,
 		[{switch_protocol, Headers, Protocol, InitialState}|_Tail]) ->
 	%% @todo This should be the last stream running otherwise we need to wait before switching.
 	%% @todo If there's streams opened after this one, fail instead of 101.
@@ -1076,8 +1076,11 @@ commands(State0=#state{ref=Ref, parent=Parent, socket=Socket, transport=Transpor
 	%% @todo Handle cases where the request came with a body. We need
 	%% to process or skip the body before the upgrade can be completed.
 	Transport:setopts(Socket, [{active, false}]),
-	%% Send a 101 response, then terminate the stream.
-	#state{streams=Streams} = info(State, StreamID, {inform, 101, Headers}),
+	%% Send a 101 response if necessary, then terminate the stream.
+	#state{streams=Streams} = case OutState of
+		wait -> info(State, StreamID, {inform, 101, Headers});
+		_ -> State
+	end,
 	#stream{state=StreamState} = lists:keyfind(StreamID, #stream.id, Streams),
 	%% @todo We need to shutdown processes here first.
 	stream_call_terminate(StreamID, switch_protocol, StreamState, State),

test/handlers/stream_handler_h.erl

@@ -46,6 +46,12 @@ init_commands(_, _, State=#state{test=shutdown_timeout_on_stream_stop}) ->
 init_commands(_, _, State=#state{test=shutdown_timeout_on_socket_close}) ->
 	Spawn = init_process(true, State),
 	[{headers, 200, #{}}, {spawn, Spawn, 2000}];
+init_commands(_, _, State=#state{test=switch_protocol_after_headers}) ->
+	[{headers, 200, #{}}, {switch_protocol, #{}, ?MODULE, State}];
+init_commands(_, _, State=#state{test=switch_protocol_after_headers_data}) ->
+	[{headers, 200, #{}}, {data, fin, <<"{}">>}, {switch_protocol, #{}, ?MODULE, State}];
+init_commands(_, _, State=#state{test=switch_protocol_after_response}) ->
+	[{response, 200, #{}, <<"{}">>}, {switch_protocol, #{}, ?MODULE, State}];
 init_commands(_, _, State=#state{test=terminate_on_switch_protocol}) ->
 	[{switch_protocol, #{}, ?MODULE, State}];
 init_commands(_, _, #state{test=terminate_on_stop}) ->

test/stream_handler_SUITE.erl

@@ -325,6 +325,54 @@ shutdown_timeout_on_socket_close(Config) ->
 	receive {'DOWN', MRef, process, Spawn, killed} -> ok after 1000 -> error(timeout) end,
 	ok.
 
+switch_protocol_after_headers(Config) ->
+	case config(protocol, Config) of
+		http -> do_switch_protocol_after_response(
+			<<"switch_protocol_after_headers">>, Config);
+		http2 -> doc("The switch_protocol command is not currently supported for HTTP/2.")
+	end.
+
+switch_protocol_after_headers_data(Config) ->
+	case config(protocol, Config) of
+		http -> do_switch_protocol_after_response(
+			<<"switch_protocol_after_headers_data">>, Config);
+		http2 -> doc("The switch_protocol command is not currently supported for HTTP/2.")
+	end.
+
+switch_protocol_after_response(Config) ->
+	case config(protocol, Config) of
+		http -> do_switch_protocol_after_response(
+			<<"switch_protocol_after_response">>, Config);
+		http2 -> doc("The switch_protocol command is not currently supported for HTTP/2.")
+	end.
+
+do_switch_protocol_after_response(TestCase, Config) ->
+	doc("The 101 informational response must not be sent when a response "
+		"has already been sent before the switch_protocol is returned."),
+	Self = self(),
+	ConnPid = gun_open(Config),
+	Ref = gun:get(ConnPid, "/long_polling", [
+		{<<"accept-encoding">>, <<"gzip">>},
+		{<<"x-test-case">>, TestCase},
+		{<<"x-test-pid">>, pid_to_list(Self)}
+	]),
+	%% Confirm init/3 is called and receive the response.
+	Pid = receive {Self, P, init, _, _, _} -> P after 1000 -> error(timeout) end,
+	{response, nofin, 200, _} = gun:await(ConnPid, Ref),
+	case TestCase of
+		<<"switch_protocol_after_headers">> ->
+			ok;
+		_ ->
+			{ok, <<"{}">>} = gun:await_body(ConnPid, Ref),
+			ok
+	end,
+	{error, _} = gun:await(ConnPid, Ref),
+	%% Confirm terminate/3 is called.
+	receive {Self, Pid, terminate, _, _, _} -> ok after 1000 -> error(timeout) end,
+	%% Confirm takeover/7 is called.
+	receive {Self, Pid, takeover, _, _, _, _, _, _, _} -> ok after 1000 -> error(timeout) end,
+	ok.
+
 terminate_on_socket_close(Config) ->
 	doc("Confirm terminate/3 is called when the socket gets closed brutally."),
 	Self = self(),