Add SETTINGS ack timeout and option settings_timeout

doc/src/manual/cowboy_http2.asciidoc

@@ -30,6 +30,7 @@ opts() :: #{
     max_frame_size_sent            => 16384..16777215 | infinity,
     middlewares                    => [module()],
     preface_timeout                => timeout(),
+    settings_timeout               => timeout(),
     shutdown_timeout               => timeout(),
     stream_handlers                => [module()]
 }
@@ -105,6 +106,9 @@ middlewares ([cowboy_router, cowboy_handler])::
 preface_timeout (5000)::
     Time in ms Cowboy is willing to wait for the connection preface.
 
+settings_timeout (5000)::
+    Time in ms Cowboy is willing to wait for a SETTINGS ack.
+
 shutdown_timeout (5000)::
     Time in ms Cowboy will wait for child processes to shut down before killing them.
 
@@ -116,8 +120,9 @@ stream_handlers ([cowboy_stream_h])::
 * *2.4*: Add the options `initial_connection_window_size`,
          `initial_stream_window_size`, `max_concurrent_streams`,
          `max_decode_table_size`, `max_encode_table_size`,
-         `max_frame_size_received` and `max_frame_size_sent`
-         to configure HTTP/2 SETTINGS.
+         `max_frame_size_received`, `max_frame_size_sent`
+         and `settings_timeout` to configure HTTP/2 SETTINGS
+         and related behavior.
 * *2.4*: Add the experimental option `enable_connect_protocol`.
 * *2.0*: Protocol introduced.
 

src/cowboy_http2.erl

@@ -36,6 +36,7 @@
 	max_frame_size_sent => 16384..16777215 | infinity,
 	middlewares => [module()],
 	preface_timeout => timeout(),
+	settings_timeout => timeout(),
 	shutdown_timeout => timeout(),
 	stream_handlers => [module()]
 }.
@@ -85,10 +86,6 @@
 
 	%% Settings are separate for each endpoint. In addition, settings
 	%% must be acknowledged before they can be expected to be applied.
-	%%
-	%% @todo Since the ack is required, we must timeout if we don't receive it.
-	%% @todo I haven't put as much thought as I should have on this,
-	%% the final settings handling will be very different.
 	local_settings = #{
 %		header_table_size => 4096,
 %		enable_push => false, %% We are the server. Push is never enabled for clients.
@@ -97,10 +94,8 @@
 		max_frame_size => 16384
 %		max_header_list_size => infinity
 	} :: map(),
-	%% @todo We need a TimerRef to do SETTINGS_TIMEOUT errors.
-	%% We need to be careful there. It's well possible that we send
-	%% two SETTINGS frames before we receive a SETTINGS ack.
 	next_settings = undefined :: undefined | map(),
+	next_settings_timer = undefined :: undefined | reference(),
 	remote_settings = #{
 		initial_window_size => 65535
 	} :: map(),
@@ -228,7 +223,13 @@ settings_init(State, Opts) ->
 	%% @todo max_header_list_size
 	Settings = setting_from_opt(S3, Opts, enable_connect_protocol,
 		enable_connect_protocol, false),
-	State#state{next_settings=Settings}.
+	%% Start a timer if necessary. The timer will trigger only
+	%% if no SETTINGS ack was received in time.
+	TRef = case maps:get(settings_timeout, Opts, 5000) of
+		infinity -> undefined;
+		SettingsTimeout -> erlang:start_timer(SettingsTimeout, self(), settings_timeout)
+	end,
+	State#state{next_settings=Settings, next_settings_timer=TRef}.
 
 setting_from_opt(Settings, Opts, OptName, SettingName, Default) ->
 	case maps:get(OptName, Opts, Default) of
@@ -258,7 +259,8 @@ before_loop(State, Buffer) ->
 	loop(State, Buffer).
 
 loop(State=#state{parent=Parent, socket=Socket, transport=Transport,
-		opts=Opts, children=Children, parse_state=PS}, Buffer) ->
+		opts=Opts, children=Children, next_settings_timer=SettingsTRef,
+		parse_state=PS}, Buffer) ->
 	Transport:setopts(Socket, [{active, once}]),
 	{OK, Closed, Error} = Transport:messages(),
 	InactivityTimeout = maps:get(inactivity_timeout, Opts, 300000),
@@ -288,6 +290,9 @@ loop(State=#state{parent=Parent, socket=Socket, transport=Transport,
 				_ ->
 					loop(State, Buffer)
 			end;
+		{timeout, SettingsTRef, settings_timeout} ->
+			terminate(State, {connection_error, settings_timeout,
+				'The SETTINGS ack was not received within the configured time. (RFC7540 6.5.3)'});
 		%% Messages pertaining to a stream.
 		{{Pid, StreamID}, Msg} when Pid =:= self() ->
 			loop(info(State, StreamID, Msg), Buffer);
@@ -470,9 +475,12 @@ frame(State0=#state{socket=Socket, transport=Transport, opts=Opts,
 			State
 	end, State1, Settings);
 %% Ack for a previously sent SETTINGS frame.
-frame(State0=#state{local_settings=Local0, next_settings=NextSettings}, settings_ack) ->
+frame(State0=#state{local_settings=Local0, next_settings=NextSettings,
+		next_settings_timer=TRef}, settings_ack) ->
+	ok = erlang:cancel_timer(TRef, [{async, true}, {info, false}]),
 	Local = maps:merge(Local0, NextSettings),
-	State1 = State0#state{local_settings=Local, next_settings=#{}},
+	State1 = State0#state{local_settings=Local, next_settings=#{},
+		next_settings_timer=undefined},
 	maps:fold(fun
 		(header_table_size, MaxSize, State=#state{decode_state=DecodeState0}) ->
 			DecodeState = cow_hpack:set_max_size(MaxSize, DecodeState0),

test/rfc7540_SUITE.erl

@@ -420,11 +420,32 @@ http_upgrade_client_preface_settings_ack_timeout(Config) ->
 	%% Receive the server preface.
 	{ok, << Len:24 >>} = gen_tcp:recv(Socket, 3, 1000),
 	{ok, << 4:8, 0:40, _:Len/binary >>} = gen_tcp:recv(Socket, 6 + Len, 1000),
-	%% Receive the SETTINGS ack.
-	{ok, << 0:24, 4:8, 1:8, 0:32 >>} = gen_tcp:recv(Socket, 9, 1000),
-	%% Do not ack the server preface. Expect a GOAWAY with reason SETTINGS_TIMEOUT.
-	{ok, << _:24, 7:8, _:72, 4:32 >>} = gen_tcp:recv(Socket, 17, 6000),
-	ok.
+	%% Skip the SETTINGS ack. Receive a GOAWAY with reason SETTINGS_TIMEOUT,
+	%% possibly following a HEADERS or HEADERS and DATA frames.
+	Received = lists:reverse(lists:foldl(fun(_, Acc) ->
+		case gen_tcp:recv(Socket, 9, 6000) of
+			{ok, << 0:24, 4:8, 1:8, 0:32 >>} ->
+				Acc;
+			{ok, << SkipLen:24, 1:8, _:8, 1:32 >>} ->
+				{ok, _} = gen_tcp:recv(Socket, SkipLen, 1000),
+				[headers|Acc];
+			{ok, << SkipLen:24, 0:8, _:8, 1:32 >>} ->
+				{ok, _} = gen_tcp:recv(Socket, SkipLen, 1000),
+				[data|Acc];
+			{ok, << 8:24, 7:8, 0:40 >>} ->
+				%% We expect a SETTINGS_TIMEOUT reason.
+				{ok, << 1:32, 4:32 >>} = gen_tcp:recv(Socket, 8, 1000),
+				[goaway|Acc];
+			{error, _} ->
+				%% Can be timeouts, ignore them.
+				Acc
+		end
+	end, [], [1, 2, 3, 4])),
+	case Received of
+		[goaway] -> ok;
+		[headers, goaway] -> ok;
+		[headers, data, goaway] -> ok
+	end.
 
 %% @todo We need a successful test with actual options in HTTP2-Settings.
 %% SETTINGS_MAX_FRAME_SIZE is probably the easiest to test. The relevant