From 2db5ffbf842e0aeb0ae11484b8c247bbfdcf5e9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Hoguin?= Date: Sat, 28 Apr 2018 10:59:56 +0200 Subject: Add SETTINGS ack timeout and option settings_timeout --- doc/src/manual/cowboy_http2.asciidoc | 9 +++++++-- src/cowboy_http2.erl | 30 +++++++++++++++++++----------- test/rfc7540_SUITE.erl | 31 ++++++++++++++++++++++++++----- 3 files changed, 52 insertions(+), 18 deletions(-) diff --git a/doc/src/manual/cowboy_http2.asciidoc b/doc/src/manual/cowboy_http2.asciidoc index 47aa012..cd8ca68 100644 --- a/doc/src/manual/cowboy_http2.asciidoc +++ b/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. diff --git a/src/cowboy_http2.erl b/src/cowboy_http2.erl index b2f79e3..9bd1bda 100644 --- a/src/cowboy_http2.erl +++ b/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), diff --git a/test/rfc7540_SUITE.erl b/test/rfc7540_SUITE.erl index 5b16294..85e83a5 100644 --- a/test/rfc7540_SUITE.erl +++ b/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 -- cgit v1.2.3