From fedea1fd8d57a6188d9ca82968ee3eb841234c68 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 20 Nov 2023 14:55:04 +0100 Subject: [PATCH 1/9] fix: adjust esockd_acceptor restart intensity and period --- src/esockd_acceptor_sup.erl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/esockd_acceptor_sup.erl b/src/esockd_acceptor_sup.erl index 1ed2861..e763b5b 100644 --- a/src/esockd_acceptor_sup.erl +++ b/src/esockd_acceptor_sup.erl @@ -55,8 +55,8 @@ count_acceptors(AcceptorSup) -> init([Proto, ListenOn, ConnSup, TuneFun, UpgradeFuns, Limiter]) -> SupFlags = #{strategy => simple_one_for_one, - intensity => 100, - period => 3600 + intensity => 100000, + period => 1 }, Acceptor = #{id => acceptor, start => {esockd_acceptor, start_link, From 526a8e5e9b9220ba35a63240aede9768eb81cfb0 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 20 Nov 2023 14:55:55 +0100 Subject: [PATCH 2/9] fix: listener handle port close --- src/esockd_listener.erl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/esockd_listener.erl b/src/esockd_listener.erl index 60f1ec3..4e6ff53 100644 --- a/src/esockd_listener.erl +++ b/src/esockd_listener.erl @@ -117,6 +117,9 @@ handle_cast(Msg, State) -> error_logger:error_msg("[~s] Unexpected cast: ~p", [?MODULE, Msg]), {noreply, State}. +handle_info({'EXIT', LSock, _}, #state{lsock = LSock} = State) -> + error_logger:error_msg("~s Lsocket ~p closed", [?MODULE, LSock]), + {stop, lsock_closed, State}; handle_info(Info, State) -> error_logger:error_msg("[~s] Unexpected info: ~p", [?MODULE, Info]), {noreply, State}. From ce22d1aa476c6b558e0930bab94e373327b334e3 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 20 Nov 2023 16:17:52 +0100 Subject: [PATCH 3/9] test: listener handle port close --- src/esockd_listener.erl | 8 ++++++++ test/esockd_SUITE.erl | 28 ++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/src/esockd_listener.erl b/src/esockd_listener.erl index 4e6ff53..07c86aa 100644 --- a/src/esockd_listener.erl +++ b/src/esockd_listener.erl @@ -24,6 +24,7 @@ -export([ options/1 , get_port/1 + , get_lsock/1 ]). %% gen_server callbacks @@ -65,6 +66,10 @@ options(Listener) -> get_port(Listener) -> gen_server:call(Listener, get_port). +-spec get_lsock(pid()) -> inet:socket(). +get_lsock(Listener) -> + gen_server:call(Listener, get_lsock). + %%-------------------------------------------------------------------- %% gen_server callbacks %%-------------------------------------------------------------------- @@ -109,6 +114,9 @@ handle_call(options, _From, State = #state{options = Opts}) -> handle_call(get_port, _From, State = #state{lport = LPort}) -> {reply, LPort, State}; +handle_call(get_lsock, _From, State = #state{lsock = LSock}) -> + {reply, LSock, State}; + handle_call(Req, _From, State) -> error_logger:error_msg("[~s] Unexpected call: ~p", [?MODULE, Req]), {noreply, State}. diff --git a/test/esockd_SUITE.erl b/test/esockd_SUITE.erl index f1b0065..e7988a1 100644 --- a/test/esockd_SUITE.erl +++ b/test/esockd_SUITE.erl @@ -416,6 +416,34 @@ t_tune_fun_ok(_) -> ?assertEqual(1, proplists:get_value(accepted, Cnts)), ok = esockd:close(Name, LPort). +t_listener_handle_port_exit(_) -> + LPort = 7005, + Name = ?FUNCTION_NAME, + %% GIVEN: when listener is started + {ok, LSup} = esockd:open(Name, LPort, [], + {echo_server, start_link, []}), + L = esockd_listener_sup:listener(LSup), + PortInUse = esockd_listener:get_port(L), + ?assertEqual(LPort, PortInUse), + LSock = esockd_listener:get_lsock(L), + erlang:process_flag(trap_exit, true), + link(L), + %% GIVEN: when port is closed + erlang:port_close(LSock), + %% THEN: listener process should EXIT, ABNORMALLY + receive + {'EXIT', L, lsock_closed} -> + ok + after 300 -> + ct:fail(listener_still_alive) + end, + %% THEN: new listener should be restarted + NewListener = esockd_listener_sup:listener(LSup), + ?assertNotEqual(L, NewListener), + ?assertEqual(PortInUse, esockd_listener:get_port(NewListener)), + ?assertMatch({error, eaddrinuse}, gen_tcp:listen(PortInUse, [])), + ok = esockd:close(Name, LPort). + %% helper sock_tune_fun(Ret) -> Ret. From 863006330795c2fbbef4be030fbfe7ba20016868 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 20 Nov 2023 20:42:08 +0100 Subject: [PATCH 4/9] test: update test for ports close --- test/esockd_SUITE.erl | 57 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 53 insertions(+), 4 deletions(-) diff --git a/test/esockd_SUITE.erl b/test/esockd_SUITE.erl index e7988a1..cb86007 100644 --- a/test/esockd_SUITE.erl +++ b/test/esockd_SUITE.erl @@ -416,11 +416,29 @@ t_tune_fun_ok(_) -> ?assertEqual(1, proplists:get_value(accepted, Cnts)), ok = esockd:close(Name, LPort). -t_listener_handle_port_exit(_) -> + +t_listener_handle_port_exit_tcp(Config) -> + do_listener_handle_port_exit(Config, false). + +t_listener_handle_port_exit_tls(Config) -> + do_listener_handle_port_exit(Config, true). + +do_listener_handle_port_exit(Config, IsTls) -> LPort = 7005, Name = ?FUNCTION_NAME, + SslOpts = [{certfile, esockd_ct:certfile(Config)}, + {keyfile, esockd_ct:keyfile(Config)}, + {gc_after_handshake, true}, + {verify, verify_none} + ], + OpenOpts = case IsTls of + true -> + ssl:start(), + [{ssl_options, SslOpts}]; + false -> [] + end, %% GIVEN: when listener is started - {ok, LSup} = esockd:open(Name, LPort, [], + {ok, LSup} = esockd:open(Name, LPort, OpenOpts, {echo_server, start_link, []}), L = esockd_listener_sup:listener(LSup), PortInUse = esockd_listener:get_port(L), @@ -428,7 +446,22 @@ t_listener_handle_port_exit(_) -> LSock = esockd_listener:get_lsock(L), erlang:process_flag(trap_exit, true), link(L), - %% GIVEN: when port is closed + Acceptors = get_acceptors(LSup), + ?assertNotEqual([], Acceptors), + + case IsTls of + true -> + {ok, ClientSock} = ssl:connect("localhost", LPort, [{verify, verify_none} + ], 1000), + ssl:close(ClientSock); + false -> + {ok, ClientSock} = gen_tcp:connect("localhost", LPort, [], 1000), + ok = gen_tcp:close(ClientSock) + end, + + timer:sleep(100), + + %% WHEN: when port is closed erlang:port_close(LSock), %% THEN: listener process should EXIT, ABNORMALLY receive @@ -437,13 +470,29 @@ t_listener_handle_port_exit(_) -> after 300 -> ct:fail(listener_still_alive) end, - %% THEN: new listener should be restarted + + %% THEN: listener should be restarted NewListener = esockd_listener_sup:listener(LSup), ?assertNotEqual(L, NewListener), + + %% THEN: listener should be listening on the same port ?assertEqual(PortInUse, esockd_listener:get_port(NewListener)), ?assertMatch({error, eaddrinuse}, gen_tcp:listen(PortInUse, [])), + + %% THEN: New acceptors should be started with new LSock to accept, + %% (old acceptors should be terminated due to `closed') + NewAcceptors = get_acceptors(LSup), + ?assertNotEqual([], NewAcceptors), + ?assert(sets:is_empty(sets:intersection(sets:from_list(Acceptors), sets:from_list(NewAcceptors)))), + ok = esockd:close(Name, LPort). %% helper sock_tune_fun(Ret) -> Ret. + +-spec get_acceptors(supervisor:supervisor()) -> [Acceptor::pid()]. +get_acceptors(LSup) -> + Children = supervisor:which_children(LSup), + {acceptor_sup, AcceptorSup, _, _} = lists:keyfind(acceptor_sup, 1, Children), + supervisor:which_children(AcceptorSup). From 87640ea7e4b1b9974fbc932dcc135cda6ad2f8c0 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 20 Nov 2023 21:12:24 +0100 Subject: [PATCH 5/9] fix(acceptor): don't close listening socket in terminate as it spreads error to the whole sup tree --- src/esockd_acceptor.erl | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/esockd_acceptor.erl b/src/esockd_acceptor.erl index 76ea6e1..2128f4f 100644 --- a/src/esockd_acceptor.erl +++ b/src/esockd_acceptor.erl @@ -137,6 +137,7 @@ handle_event(internal, begin_waiting, waiting, State = #state{lsock = LSock}) -> {error, closed} -> {stop, normal, State}; {error, Reason} -> + error_logger:error_msg("~p async_accept error: ~p", [?MODULE, Reason]), {stop, Reason, State} end; handle_event( @@ -210,8 +211,11 @@ handle_event(Type, Content, StateName, _) -> ), keep_state_and_data. -terminate(_Reason, _StateName, #state{lsock = LSock}) -> - close(LSock). +terminate(normal, _StateName, #state{}) -> + ok; +terminate(Reason, _StateName, #state{}) -> + error_logger:error_msg("~p terminating due to ~p", [?MODULE, Reason]), + ok. code_change(_OldVsn, StateName, State, _Extra) -> {ok, StateName, State}. From af38485bbc81cd0835d4d672e29f054ab9f748cb Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 21 Nov 2023 09:05:30 +0100 Subject: [PATCH 6/9] ci: coverall only when success --- .github/workflows/run_test_case.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/run_test_case.yaml b/.github/workflows/run_test_case.yaml index 455b76a..c201de7 100644 --- a/.github/workflows/run_test_case.yaml +++ b/.github/workflows/run_test_case.yaml @@ -31,6 +31,7 @@ jobs: make ct make cover - name: Coveralls + if: ${{ success() }} env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} run: | From 455a93bfd0303084f42aab44aac123fef2b632cf Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 21 Nov 2023 10:16:35 +0100 Subject: [PATCH 7/9] ci: move coveralls to seprate job --- .github/workflows/run_test_case.yaml | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/.github/workflows/run_test_case.yaml b/.github/workflows/run_test_case.yaml index c201de7..f08cbe0 100644 --- a/.github/workflows/run_test_case.yaml +++ b/.github/workflows/run_test_case.yaml @@ -30,14 +30,21 @@ jobs: make eunit make ct make cover - - name: Coveralls - if: ${{ success() }} - env: - GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - run: | - make coveralls - uses: actions/upload-artifact@v1 if: failure() with: name: logs path: _build/test/logs + + coveralls: + runs-on: ubuntu-latest + container: + image: erlang:25 + steps: + - uses: actions/checkout@v1 + - name: Coveralls + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + run: | + rebar3 as test do eunit,ct,cover + make coveralls From 40356b18f9d36c05cb8bcf5fb5c2587ed268ba4f Mon Sep 17 00:00:00 2001 From: William Yang Date: Wed, 22 Nov 2023 12:38:51 +0100 Subject: [PATCH 8/9] fix: ensure counters when listener restarts by sup --- src/esockd_dtls_listener.erl | 1 + src/esockd_listener.erl | 1 + src/esockd_listener_sup.erl | 3 --- src/esockd_server.erl | 5 +++++ 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/src/esockd_dtls_listener.erl b/src/esockd_dtls_listener.erl index 19e720e..7fadfe3 100644 --- a/src/esockd_dtls_listener.erl +++ b/src/esockd_dtls_listener.erl @@ -70,6 +70,7 @@ get_port(Listener) -> init({Proto, ListenOn, Opts, AcceptorSup}) -> Port = port(ListenOn), process_flag(trap_exit, true), + esockd_server:ensure_stats({Proto, ListenOn}), SockOpts = merge_addr(ListenOn, dltsopts(Opts)), %% Don't active the socket... case ssl:listen(Port, SockOpts) of diff --git a/src/esockd_listener.erl b/src/esockd_listener.erl index 07c86aa..19ac7d8 100644 --- a/src/esockd_listener.erl +++ b/src/esockd_listener.erl @@ -77,6 +77,7 @@ get_lsock(Listener) -> init({Proto, ListenOn, Opts, AcceptorSup}) -> Port = port(ListenOn), process_flag(trap_exit, true), + esockd_server:ensure_stats({Proto, ListenOn}), SockOpts = merge_addr(ListenOn, sockopts(Opts)), %% Don't active the socket... case esockd_transport:listen(Port, [{active, false} | proplists:delete(active, SockOpts)]) of diff --git a/src/esockd_listener_sup.erl b/src/esockd_listener_sup.erl index 28fbb23..5063c3c 100644 --- a/src/esockd_listener_sup.erl +++ b/src/esockd_listener_sup.erl @@ -73,10 +73,7 @@ start_link(Type, Proto, ListenOn, Opts, MFA) -> type => supervisor, modules => [esockd_connection_sup]}, {ok, ConnSup} = supervisor:start_child(Sup, ConnSupSpec), - %% Start acceptor sup - ok = esockd_server:init_stats({Proto, ListenOn}, accepted), - ok = esockd_server:init_stats({Proto, ListenOn}, closed_overloaded), TuneFun = tune_socket_fun(Opts), UpgradeFuns = upgrade_funs(Type, Opts), Limiter = conn_rate_limiter(conn_limiter_opts(Opts, {listener, Proto, ListenOn})), diff --git a/src/esockd_server.erl b/src/esockd_server.erl index 4c56fb3..f1ffc19 100644 --- a/src/esockd_server.erl +++ b/src/esockd_server.erl @@ -27,6 +27,7 @@ , inc_stats/3 , dec_stats/3 , del_stats/1 + , ensure_stats/1 ]). %% gen_server callbacks @@ -85,6 +86,10 @@ update_counter(Key, Num) -> del_stats({Protocol, ListenOn}) -> gen_server:cast(?SERVER, {del, {Protocol, ListenOn}}). +-spec ensure_stats({atom(), esockd:listen_on()}) -> ok. +ensure_stats(StatsKey) -> + ok = ?MODULE:init_stats(StatsKey, accepted), + ok = ?MODULE:init_stats(StatsKey, closed_overloaded). %%-------------------------------------------------------------------- %% gen_server callbacks %%-------------------------------------------------------------------- From cbf63b52363ba527f8712c1cd22155e0f36733f4 Mon Sep 17 00:00:00 2001 From: William Yang Date: Wed, 22 Nov 2023 13:19:07 +0100 Subject: [PATCH 9/9] fix: handle econnaborted return from async_accept --- src/esockd_acceptor.erl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/esockd_acceptor.erl b/src/esockd_acceptor.erl index 2128f4f..626c559 100644 --- a/src/esockd_acceptor.erl +++ b/src/esockd_acceptor.erl @@ -134,6 +134,8 @@ handle_event(internal, begin_waiting, waiting, State = #state{lsock = LSock}) -> Reason =:= enfile -> {next_state, suspending, State, {state_timeout, 1000, begin_waiting}}; + {error, econnaborted} -> + {next_state, waiting, State, {next_event, internal, begin_waiting}}; {error, closed} -> {stop, normal, State}; {error, Reason} ->