diff --git a/NEWS.md b/NEWS.md index 6845e6a8..579ce350 100644 --- a/NEWS.md +++ b/NEWS.md @@ -18,6 +18,13 @@ UNRELEASED ### Bug Fixes +- Wire `hackney_altsvc:parse_and_cache/3` into the response path so + server-advertised HTTP/3 endpoints are actually recorded. Previously + the cache was only populated by manual `cache/4` calls; the HTTP/3 + guide claimed automatic discovery but it never fired. Same hook + honors RFC 7838 `clear` (invalidates the cached entry) and merges + multiple `Alt-Svc` headers per RFC 7230 §3.2.2. Fires on every + protocol so the cache TTL stays fresh while h3 is in use. - Fix HTTP/2 pooled connections wedging under sustained concurrent load (#836). The pool checks out a TCP connection first then upgrades to SSL+ALPN; `connected(enter)` armed the 2s pool idle timer while the diff --git a/src/hackney_altsvc.erl b/src/hackney_altsvc.erl index 22dd55c0..9d4c22ac 100644 --- a/src/hackney_altsvc.erl +++ b/src/hackney_altsvc.erl @@ -80,22 +80,34 @@ parse(Header) when is_binary(Header) -> parse_entries(Header, []). %% @doc Parse Alt-Svc header from response headers and cache if h3 found. -%% Returns {ok, h3, Port} if HTTP/3 is available, or none. +%% +%% Honors RFC 7838: a `clear' value invalidates any cached entry for the +%% origin. Multiple `Alt-Svc' headers are merged before parsing as if +%% they had been comma-concatenated (RFC 7230 §3.2.2). +%% +%% Returns `{ok, h3, Port}' if HTTP/3 is now cached for the origin, +%% `cleared' if the cache was invalidated, or `none' otherwise. -spec parse_and_cache(Host :: binary() | string(), Port :: inet:port_number(), Headers :: [{binary(), binary()}]) -> - {ok, h3, inet:port_number()} | none. + {ok, h3, inet:port_number()} | cleared | none. parse_and_cache(Host, OrigPort, Headers) -> - case find_altsvc_header(Headers) of - undefined -> + case collect_altsvc_headers(Headers) of + <<>> -> none; - Value -> - Entries = parse(Value), - case find_h3_entry(Entries) of - {ok, H3Port, MaxAge} -> - cache(Host, OrigPort, H3Port, MaxAge), - {ok, h3, H3Port}; - none -> - none + Combined -> + case is_clear_directive(Combined) of + true -> + clear(Host, OrigPort), + cleared; + false -> + Entries = parse(Combined), + case find_h3_entry(Entries) of + {ok, H3Port, MaxAge} -> + cache(Host, OrigPort, H3Port, MaxAge), + {ok, h3, H3Port}; + none -> + none + end end end. @@ -174,12 +186,27 @@ make_key(Host, Port) when is_list(Host) -> make_key(Host, Port) when is_binary(Host) -> {string:lowercase(Host), Port}. -find_altsvc_header([]) -> - undefined; -find_altsvc_header([{Key, Value} | Rest]) -> - case string:lowercase(Key) of - <<"alt-svc">> -> Value; - _ -> find_altsvc_header(Rest) +%% Collect every alt-svc header value and join with ", " so multiple +%% header lines are parsed as a single combined entry list per RFC 7230. +collect_altsvc_headers(Headers) -> + Values = [to_binary(V) || {K, V} <- Headers, + string:lowercase(to_binary(K)) =:= <<"alt-svc">>], + join_with_comma(Values). + +join_with_comma([]) -> <<>>; +join_with_comma([V]) -> V; +join_with_comma([V | Rest]) -> + iolist_to_binary([V, ", ", join_with_comma(Rest)]). + +to_binary(B) when is_binary(B) -> B; +to_binary(L) when is_list(L) -> iolist_to_binary(L). + +%% RFC 7838 §3: a value of "clear" (case-insensitive) invalidates the +%% origin's cached alternatives. Surrounding whitespace is allowed. +is_clear_directive(Value) -> + case string:lowercase(string:trim(Value)) of + <<"clear">> -> true; + _ -> false end. find_h3_entry([]) -> diff --git a/src/hackney_conn.erl b/src/hackney_conn.erl index 02a96595..aae45959 100644 --- a/src/hackney_conn.erl +++ b/src/hackney_conn.erl @@ -1196,6 +1196,7 @@ receiving(internal, do_recv_response_async, Data) -> case recv_status_and_headers(DataWithParser) of {ok, Status, Headers, NewData} -> HeadersList = hackney_headers:to_list(Headers), + maybe_record_altsvc(HeadersList, NewData), %% Check if this is a redirect and we should handle it case maybe_handle_async_redirect(Status, Method, Headers, FollowRedirect) of {redirect, Location} -> @@ -1656,6 +1657,7 @@ do_recv_response_impl(Data, IncludePid) -> {ok, Status, Headers, NewData} -> From = NewData#conn_data.request_from, HeadersList = hackney_headers:to_list(Headers), + maybe_record_altsvc(HeadersList, NewData), Reply = case IncludePid of true -> {ok, Status, HeadersList, self()}; false -> {ok, Status, HeadersList} @@ -2485,6 +2487,7 @@ handle_h2_event(_Other, Data) -> {keep_state, Data}. h2_on_response(StreamId, Status, Headers, Data) -> + maybe_record_altsvc(Headers, Data), #conn_data{h2_streams = Streams} = Data, case maps:get(StreamId, Streams, undefined) of {From, {sync, waiting_headers}} -> @@ -2815,6 +2818,7 @@ handle_h3_headers(StreamId, Headers, Fin, Streams, Data) -> {ok, Status, RespHeaders} -> %% RespHeaders from hackney_h3:parse_response_headers is already a list HeadersList = RespHeaders, + maybe_record_altsvc(HeadersList, Data), case maps:get(StreamId, Streams, undefined) of {From, waiting_headers} -> case Fin of @@ -3083,3 +3087,14 @@ handle_h3_termination(Error, Data) -> {next_state, closed, NewData, [{reply, From, {error, Error}} | Actions]} end end. + +%% @private Hand response headers to the Alt-Svc cache so server-advertised +%% HTTP/3 endpoints get recorded for future requests. Fires for every +%% protocol so the cache TTL stays fresh while h3 is in use and `clear' +%% directives are honored even on h3 responses. +maybe_record_altsvc(Headers, #conn_data{host = Host, port = Port}) + when is_list(Headers) -> + _ = catch hackney_altsvc:parse_and_cache(Host, Port, Headers), + ok; +maybe_record_altsvc(_Headers, _Data) -> + ok. diff --git a/test/hackney_altsvc_tests.erl b/test/hackney_altsvc_tests.erl index fac58e79..1a25ef7c 100644 --- a/test/hackney_altsvc_tests.erl +++ b/test/hackney_altsvc_tests.erl @@ -79,7 +79,13 @@ cache_test_() -> {"cache and lookup", fun test_cache_lookup/0}, {"lookup returns none when not cached", fun test_lookup_none/0}, {"clear removes entry", fun test_clear/0}, - {"parse_and_cache from headers", fun test_parse_and_cache/0} + {"parse_and_cache from headers", fun test_parse_and_cache/0}, + {"clear directive invalidates cached entry", + fun test_parse_and_cache_clear/0}, + {"multiple Alt-Svc headers are merged", + fun test_parse_and_cache_multi_header/0}, + {"clear directive case-insensitive with whitespace", + fun test_parse_and_cache_clear_loose/0} ] } }. @@ -106,6 +112,31 @@ test_parse_and_cache() -> %% Verify it's cached ?assertEqual({ok, h3, 443}, hackney_altsvc:lookup(<<"cached.example.com">>, 443)). +test_parse_and_cache_clear() -> + Host = <<"clear-via-header.example.com">>, + hackney_altsvc:cache(Host, 443, 8443, 3600), + ?assertEqual({ok, h3, 8443}, hackney_altsvc:lookup(Host, 443)), + Headers = [{<<"alt-svc">>, <<"clear">>}], + ?assertEqual(cleared, + hackney_altsvc:parse_and_cache(Host, 443, Headers)), + ?assertEqual(none, hackney_altsvc:lookup(Host, 443)). + +test_parse_and_cache_multi_header() -> + %% Two separate Alt-Svc headers must be treated as one combined list. + Headers = [{<<"alt-svc">>, <<"h2=\":443\"">>}, + {<<"alt-svc">>, <<"h3=\":8443\"; ma=600">>}], + Result = hackney_altsvc:parse_and_cache(<<"multi.example.com">>, 443, Headers), + ?assertEqual({ok, h3, 8443}, Result), + ?assertEqual({ok, h3, 8443}, hackney_altsvc:lookup(<<"multi.example.com">>, 443)). + +test_parse_and_cache_clear_loose() -> + Host = <<"clear-loose.example.com">>, + hackney_altsvc:cache(Host, 443, 443, 3600), + Headers = [{<<"Alt-Svc">>, <<" CLEAR ">>}], + ?assertEqual(cleared, + hackney_altsvc:parse_and_cache(Host, 443, Headers)), + ?assertEqual(none, hackney_altsvc:lookup(Host, 443)). + %%==================================================================== %% Blocked Cache Tests %%==================================================================== diff --git a/test/hackney_altsvc_wiring_tests.erl b/test/hackney_altsvc_wiring_tests.erl new file mode 100644 index 00000000..e940d5e9 --- /dev/null +++ b/test/hackney_altsvc_wiring_tests.erl @@ -0,0 +1,87 @@ +%%% -*- erlang -*- +%%% +%%% This file is part of hackney released under the Apache 2 license. +%%% See the NOTICE for more information. +%%% +%%% End-to-end test that response-side Alt-Svc parsing actually fires. +%%% Stands up a tiny cowboy listener that emits a chosen Alt-Svc header +%%% and asserts the cache reflects what the server said. +-module(hackney_altsvc_wiring_tests). +-include_lib("eunit/include/eunit.hrl"). + +-export([init/2]). %% cowboy handler callback + +-define(PORT, 9982). + +setup() -> + {ok, _} = application:ensure_all_started(hackney), + {ok, _} = application:ensure_all_started(cowboy), + Dispatch = cowboy_router:compile([ + {'_', [{"/[...]", ?MODULE, []}]} + ]), + {ok, _} = cowboy:start_clear(altsvc_test_http, + [{port, ?PORT}], + #{env => #{dispatch => Dispatch}}), + hackney_altsvc:init(), + hackney_altsvc:clear_all(), + ok. + +cleanup(_) -> + cowboy:stop_listener(altsvc_test_http), + hackney_altsvc:clear_all(), + ok. + +url(Path) -> + <<"http://localhost:", (integer_to_binary(?PORT))/binary, Path/binary>>. + +altsvc_wiring_test_() -> + {setup, fun setup/0, fun cleanup/1, + [ + fun http1_response_populates_cache/0, + fun http1_clear_directive_invalidates_cache/0, + fun absent_header_leaves_cache_alone/0 + ]}. + +%% ============================================================================ + +http1_response_populates_cache() -> + hackney_altsvc:clear_all(), + %% Cowboy handler reads the encoded Alt-Svc value from ?value= and + %% emits it on the response. + URL = url(<<"/altsvc?value=", (url_encode(<<"h3=\":8443\"; ma=600">>))/binary>>), + {ok, 200, _, _} = hackney:request(get, URL, [], <<>>, [{pool, false}]), + ?assertEqual({ok, h3, 8443}, + hackney_altsvc:lookup(<<"localhost">>, ?PORT)). + +http1_clear_directive_invalidates_cache() -> + hackney_altsvc:cache(<<"localhost">>, ?PORT, 8443, 3600), + ?assertEqual({ok, h3, 8443}, + hackney_altsvc:lookup(<<"localhost">>, ?PORT)), + URL = url(<<"/altsvc?value=clear">>), + {ok, 200, _, _} = hackney:request(get, URL, [], <<>>, [{pool, false}]), + ?assertEqual(none, hackney_altsvc:lookup(<<"localhost">>, ?PORT)). + +absent_header_leaves_cache_alone() -> + hackney_altsvc:cache(<<"localhost">>, ?PORT, 8443, 3600), + URL = url(<<"/noaltsvc">>), + {ok, 200, _, _} = hackney:request(get, URL, [], <<>>, [{pool, false}]), + %% Cache untouched. + ?assertEqual({ok, h3, 8443}, + hackney_altsvc:lookup(<<"localhost">>, ?PORT)). + +%% ============================================================================ +%% cowboy handler — sets Alt-Svc from ?value= query parameter, or none. +%% ============================================================================ + +init(Req0, State) -> + QS = cowboy_req:parse_qs(Req0), + Headers0 = #{<<"content-type">> => <<"text/plain">>}, + Headers = case proplists:get_value(<<"value">>, QS) of + undefined -> Headers0; + Value -> Headers0#{<<"alt-svc">> => Value} + end, + Req = cowboy_req:reply(200, Headers, <<"ok">>, Req0), + {ok, Req, State}. + +url_encode(Bin) -> + list_to_binary(uri_string:quote(binary_to_list(Bin))).