From a5388a324e7cc24ad2a4720de4caf1d73ab7cfbc Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 3 Jul 2023 12:15:38 +0200 Subject: [PATCH 1/4] feat(listener): close with timeout --- c_src/quicer_listener.c | 11 +++++++++-- src/quicer.erl | 23 +++++++++++++++++++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/c_src/quicer_listener.c b/c_src/quicer_listener.c index fd25f0cd..23a14e83 100644 --- a/c_src/quicer_listener.c +++ b/c_src/quicer_listener.c @@ -465,6 +465,7 @@ close_listener1(ErlNifEnv *env, const ERL_NIF_TERM argv[]) { QuicerListenerCTX *l_ctx; + ERL_NIF_TERM ret = ATOM_OK; if (!enif_get_resource(env, argv[0], ctx_listener_t, (void **)&l_ctx)) { return ERROR_TUPLE_2(ATOM_BADARG); @@ -473,13 +474,19 @@ close_listener1(ErlNifEnv *env, enif_mutex_lock(l_ctx->lock); HQUIC l = l_ctx->Listener; l_ctx->Listener = NULL; + + if (l_ctx->is_closed) + { + ret = ERROR_TUPLE_2(ATOM_CLOSED); + } l_ctx->is_closed = TRUE; enif_mutex_unlock(l_ctx->lock); // It is safe to close it without holding the lock // This also ensures no ongoing listener callbacks - // This is a blocking call. @TODO have async version or use dirty scheduler + // This is a blocking call. + // MsQuic->ListenerClose(l); - return ATOM_OK; + return ret; } diff --git a/src/quicer.erl b/src/quicer.erl index b2bb355e..6aa13eda 100644 --- a/src/quicer.erl +++ b/src/quicer.erl @@ -202,9 +202,28 @@ listen(ListenOn, Opts) when is_map(Opts) -> quicer_nif:listen(ListenOn, Opts). %% @doc close listener with listener handle --spec close_listener(listener_handle()) -> ok. +-spec close_listener(listener_handle()) -> ok | {error, badarg | closed}. close_listener(Listener) -> - quicer_nif:close_listener(Listener). + close_listener(Listener, 5000). + +-spec close_listener(listener_handle(), timer:time()) -> + ok | {error, badarg | closed | timeout}. +close_listener(Listener, Timeout) -> + case quicer_nif:close_listener(Listener) of + ok -> + receive + {quic, listener_stopped, Listener} -> + ok + after Timeout -> + {error, timeout} + end; + {error, closed} -> + %% already closed + %% follow OTP behavior + ok; + {error, _} = E-> + E + end. %% @doc %% Initiate New Connection (Client) From c90e20fc517da4c1a66e3438b6945656524814f9 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 3 Jul 2023 12:31:08 +0200 Subject: [PATCH 2/4] chore: fix spec --- src/quicer.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/quicer.erl b/src/quicer.erl index 6aa13eda..a42e1d3f 100644 --- a/src/quicer.erl +++ b/src/quicer.erl @@ -202,7 +202,7 @@ listen(ListenOn, Opts) when is_map(Opts) -> quicer_nif:listen(ListenOn, Opts). %% @doc close listener with listener handle --spec close_listener(listener_handle()) -> ok | {error, badarg | closed}. +-spec close_listener(listener_handle()) -> ok | {error, badarg | closed | timeout}. close_listener(Listener) -> close_listener(Listener, 5000). From 1bc26b5996592e85d67160c2fd5d84e3486eccf6 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 3 Jul 2023 13:10:26 +0200 Subject: [PATCH 3/4] fix: dialyzer --- src/quicer_nif.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/quicer_nif.erl b/src/quicer_nif.erl index 69531fd2..8d3c6d8d 100644 --- a/src/quicer_nif.erl +++ b/src/quicer_nif.erl @@ -113,7 +113,7 @@ reg_close() -> listen(_ListenOn, _Options) -> erlang:nif_error(nif_library_not_loaded). --spec close_listener(listener_handle()) -> ok. +-spec close_listener(listener_handle()) -> ok | {error, closed | badarg}. close_listener(_Listener) -> erlang:nif_error(nif_library_not_loaded). From ca76eeb353dda23b3e47937bb2f0b836e3867dd4 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 21 Aug 2023 23:56:52 +0200 Subject: [PATCH 4/4] feat: stop then start listener also fix config resource leaks in listener --- c_src/quicer_config.c | 19 +++-- c_src/quicer_ctx.c | 4 ++ c_src/quicer_ctx.h | 1 + c_src/quicer_listener.c | 149 ++++++++++++++++++++++++++++++++++------ c_src/quicer_listener.h | 7 ++ c_src/quicer_nif.c | 2 + src/quicer.erl | 33 +++++++-- src/quicer_nif.erl | 10 +++ test/quicer_SUITE.erl | 43 +++++++++--- 9 files changed, 223 insertions(+), 45 deletions(-) diff --git a/c_src/quicer_config.c b/c_src/quicer_config.c index dd4b4501..215b8962 100644 --- a/c_src/quicer_config.c +++ b/c_src/quicer_config.c @@ -726,18 +726,17 @@ encode_parm_to_eterm(ErlNifEnv *env, || QUIC_PARAM_GLOBAL_RETRY_MEMORY_PERCENT == Param))) { if (BufferLength == sizeof(uint64_t)) - { - res = SUCCESS(ETERM_UINT_64(*(uint64_t *)Buffer)); - } + { + res = SUCCESS(ETERM_UINT_64(*(uint64_t *)Buffer)); + } else if (BufferLength == sizeof(uint32_t)) - { - res = SUCCESS(ETERM_INT(*(uint32_t *)Buffer)); - } + { + res = SUCCESS(ETERM_INT(*(uint32_t *)Buffer)); + } else if (BufferLength == sizeof(uint16_t)) - { - res = SUCCESS(ETERM_INT(*(uint16_t *)Buffer)); - } - + { + res = SUCCESS(ETERM_INT(*(uint16_t *)Buffer)); + } } else if ((QUICER_PARAM_HANDLE_TYPE_CONN == Type && (QUIC_PARAM_CONN_REMOTE_ADDRESS == Param diff --git a/c_src/quicer_ctx.c b/c_src/quicer_ctx.c index 4d27f64d..4ea8e8fb 100644 --- a/c_src/quicer_ctx.c +++ b/c_src/quicer_ctx.c @@ -53,6 +53,10 @@ deinit_l_ctx(QuicerListenerCTX *l_ctx) void destroy_l_ctx(QuicerListenerCTX *l_ctx) { + // @note, Destroy config asap as it holds rundown + // ref count in registration + destroy_config_ctx(l_ctx->config_resource); + l_ctx->config_resource = NULL; enif_release_resource(l_ctx); } diff --git a/c_src/quicer_ctx.h b/c_src/quicer_ctx.h index 6ee389f0..31237bcc 100644 --- a/c_src/quicer_ctx.h +++ b/c_src/quicer_ctx.h @@ -49,6 +49,7 @@ typedef struct QuicerListenerCTX // Listener handle closed flag // false means the handle is invalid BOOLEAN is_closed; + BOOLEAN is_stopped; BOOLEAN allow_insecure; void *reserved1; void *reserved2; diff --git a/c_src/quicer_listener.c b/c_src/quicer_listener.c index 23a14e83..54e6937d 100644 --- a/c_src/quicer_listener.c +++ b/c_src/quicer_listener.c @@ -216,15 +216,6 @@ ServerListenerCallback(__unused_parm__ HQUIC Listener, case QUIC_LISTENER_EVENT_STOP_COMPLETE: env = l_ctx->env; - - // Close listener in NIF CTX leads to NULL Listener HQUIC - assert(l_ctx->Listener == NULL); - - // Dummy call to prevent leakage if handle is not NULL - // @TODO they should be removed when we support ListenerStop call - MsQuic->ListenerClose(l_ctx->Listener); - l_ctx->Listener = NULL; - enif_send(NULL, &(l_ctx->listenerPid), NULL, @@ -232,7 +223,12 @@ ServerListenerCallback(__unused_parm__ HQUIC Listener, ATOM_QUIC, ATOM_LISTENER_STOPPED, enif_make_resource(env, l_ctx))); - is_destroy = TRUE; + if (!l_ctx->Listener) + { + // @NOTE This callback is part of the listener *close* process + // Listener is already closing, we can destroy the l_ctx now. + is_destroy = TRUE; + } enif_clear_env(env); break; default: @@ -284,7 +280,6 @@ listen2(ErlNifEnv *env, __unused_parm__ int argc, const ERL_NIF_TERM argv[]) return ERROR_TUPLE_2(ATOM_BADARG); } - // Build CredConfig QUIC_CREDENTIAL_CONFIG CredConfig; CxPlatZeroMemory(&CredConfig, sizeof(QUIC_CREDENTIAL_CONFIG)); @@ -452,6 +447,8 @@ listen2(ErlNifEnv *env, __unused_parm__ int argc, const ERL_NIF_TERM argv[]) l_ctx->Listener, alpn_buffers, alpn_buffer_length, &Address))) { TP_NIF_3(start_fail, (uintptr_t)(l_ctx->Listener), Status); + MsQuic->ListenerClose(l_ctx->Listener); + l_ctx->Listener = NULL; destroy_l_ctx(l_ctx); return ERROR_TUPLE_3(ATOM_LISTENER_START_ERROR, ATOM_STATUS(Status)); } @@ -465,6 +462,7 @@ close_listener1(ErlNifEnv *env, const ERL_NIF_TERM argv[]) { QuicerListenerCTX *l_ctx; + BOOLEAN is_destroy = FALSE; ERL_NIF_TERM ret = ATOM_OK; if (!enif_get_resource(env, argv[0], ctx_listener_t, (void **)&l_ctx)) { @@ -472,21 +470,132 @@ close_listener1(ErlNifEnv *env, } enif_mutex_lock(l_ctx->lock); + if (l_ctx->is_closed) + { + enif_mutex_unlock(l_ctx->lock); + return ERROR_TUPLE_2(ATOM_CLOSED); + } HQUIC l = l_ctx->Listener; + // set before destroy_l_ctx l_ctx->Listener = NULL; - - if (l_ctx->is_closed) - { - ret = ERROR_TUPLE_2(ATOM_CLOSED); - } l_ctx->is_closed = TRUE; + + // If is_stopped, it means the listener is already stopped. + // there will be no callback for QUIC_LISTENER_EVENT_STOP_COMPLETE + // so we need to destroy the l_ctx otherwise it will leak. + is_destroy = l_ctx->is_stopped; + enif_mutex_unlock(l_ctx->lock); - // It is safe to close it without holding the lock - // This also ensures no ongoing listener callbacks - // This is a blocking call. - // MsQuic->ListenerClose(l); + if (is_destroy) + { + destroy_l_ctx(l_ctx); + } + return ret; +} + +ERL_NIF_TERM +stop_listener1(ErlNifEnv *env, + __unused_parm__ int argc, + const ERL_NIF_TERM argv[]) +{ + QuicerListenerCTX *l_ctx; + ERL_NIF_TERM ret = ATOM_OK; + BOOLEAN is_stopped = FALSE; + assert(argc == 1); + if (!enif_get_resource(env, argv[0], ctx_listener_t, (void **)&l_ctx)) + { + return ERROR_TUPLE_2(ATOM_BADARG); + } + + enif_mutex_lock(l_ctx->lock); + HQUIC l = l_ctx->Listener; + is_stopped = l_ctx->is_stopped; + l_ctx->is_stopped = TRUE; + enif_mutex_unlock(l_ctx->lock); + if (!l) + { + return ERROR_TUPLE_2(ATOM_CLOSED); + } + else if (!is_stopped) + { + // void return + MsQuic->ListenerStop(l); + } + return ret; +} + +ERL_NIF_TERM +start_listener3(ErlNifEnv *env, + __unused_parm__ int argc, + const ERL_NIF_TERM argv[]) +{ + ERL_NIF_TERM listener_handle = argv[0]; + ERL_NIF_TERM elisten_on = argv[1]; + ERL_NIF_TERM options = argv[2]; + + QuicerListenerCTX *l_ctx; + unsigned alpn_buffer_length = 0; + QUIC_BUFFER alpn_buffers[MAX_ALPN]; + QUIC_ADDR Address = {}; + int UdpPort = 0; + + // Return value + ERL_NIF_TERM ret = ATOM_OK; + QUIC_STATUS Status = QUIC_STATUS_SUCCESS; + + if (!enif_get_resource( + env, listener_handle, ctx_listener_t, (void **)&l_ctx)) + { + return ERROR_TUPLE_2(ATOM_BADARG); + } + + char listen_on[INET6_ADDRSTRLEN + 6] = { 0 }; + if (enif_get_string( + env, elisten_on, listen_on, INET6_ADDRSTRLEN + 6, ERL_NIF_LATIN1) + > 0) + { + if (!(QuicAddr4FromString(listen_on, &Address) + || QuicAddr6FromString(listen_on, &Address))) + { + return ERROR_TUPLE_2(ATOM_BADARG); + } + } + else if (enif_get_int(env, elisten_on, &UdpPort) && UdpPort >= 0) + { + QuicAddrSetFamily(&Address, QUIC_ADDRESS_FAMILY_UNSPEC); + QuicAddrSetPort(&Address, (uint16_t)UdpPort); + } + else + { + return ERROR_TUPLE_2(ATOM_BADARG); + } + + if (!load_alpn(env, &options, &alpn_buffer_length, alpn_buffers)) + { + return ERROR_TUPLE_2(ATOM_ALPN); + } + + enif_mutex_lock(l_ctx->lock); + if (!l_ctx->Listener) + { + ret = ERROR_TUPLE_2(ATOM_CLOSED); + goto exit; + } + + if (QUIC_FAILED( + Status = MsQuic->ListenerStart( + l_ctx->Listener, alpn_buffers, alpn_buffer_length, &Address))) + { + TP_NIF_3(start_fail, (uintptr_t)(l_ctx->Listener), Status); + ret = ERROR_TUPLE_3(ATOM_LISTENER_START_ERROR, ATOM_STATUS(Status)); + goto exit; + } + l_ctx->is_stopped = FALSE; + +exit: + enif_mutex_unlock(l_ctx->lock); return ret; } diff --git a/c_src/quicer_listener.h b/c_src/quicer_listener.h index 75c9f2c3..3d1d6444 100644 --- a/c_src/quicer_listener.h +++ b/c_src/quicer_listener.h @@ -25,6 +25,13 @@ QUIC_STATUS ServerListenerCallback(HQUIC Listener, QUIC_LISTENER_EVENT *Event); ERL_NIF_TERM listen2(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); + +ERL_NIF_TERM +start_listener3(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); + +ERL_NIF_TERM +stop_listener1(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); + ERL_NIF_TERM close_listener1(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index 4f321cf2..92cd3509 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -1419,6 +1419,8 @@ static ErlNifFunc nif_funcs[] = { { "reg_open", 1, registration, 0 }, { "reg_close", 0, deregistration, 0 }, { "listen", 2, listen2, 0}, + { "start_listener", 3, start_listener3, 0}, + { "stop_listener", 1, stop_listener1, 0}, { "close_listener", 1, close_listener1, 0}, { "open_connection", 0, open_connection0, 0}, { "async_connect", 3, async_connect3, 0}, diff --git a/src/quicer.erl b/src/quicer.erl index a42e1d3f..194b0629 100644 --- a/src/quicer.erl +++ b/src/quicer.erl @@ -32,7 +32,10 @@ %% Traffic APIs -export([ listen/2 + , stop_listen/1 + , start_listen/3 , close_listener/1 + , close_listener/2 , connect/4 , async_connect/3 , handshake/1 @@ -168,19 +171,39 @@ reg_open(Profile) -> reg_close() -> quicer_nif:reg_close(). --spec start_listener(Appname :: atom(), listen_on(), +-spec start_listener(Appname :: atom() | listener_handle(), listen_on(), {listener_opts(), connection_opts(), stream_opts() | user_opts()} ) -> {ok, pid()} | {error, any()}. -start_listener(AppName, Port, Options) -> +start_listener(AppName, Port, Options) when is_atom(AppName) -> quicer_listener:start_listener(AppName, Port, Options). --spec stop_listener(atom()) -> ok. -stop_listener(AppName) -> +-spec start_listen(listener_handle(), listen_on(), listen_opts()) -> + {ok, pid()} | {error, any()}. +start_listen(Listener, Port, Options) when is_list(Options)-> + start_listen(Listener, Port, maps:from_list(Options)); +start_listen(Listener, Port, Options) -> + quicer_nif:start_listener(Listener, Port, Options). + +-spec stop_listener(atom() | listener_handle()) -> ok. +stop_listener(AppName) when is_atom(AppName)-> quicer_listener:stop_listener(AppName). +-spec stop_listen(listener_handle()) -> ok. +stop_listen(Handle) -> + case quicer_nif:stop_listener(Handle) of + ok -> + receive + {quic, listener_stopped, Handle} -> + ok + end; + %% @TODO handle already stopped + {error, Reason} -> + {error, Reason} + end. + %% @doc Start listen on Port or "HOST:PORT". %% %% listener_handle() is used for accepting new connection. @@ -210,6 +233,8 @@ close_listener(Listener) -> ok | {error, badarg | closed | timeout}. close_listener(Listener, Timeout) -> case quicer_nif:close_listener(Listener) of + ok when Timeout == 0 -> + ok; ok -> receive {quic, listener_stopped, Listener} -> diff --git a/src/quicer_nif.erl b/src/quicer_nif.erl index 8d3c6d8d..c682876f 100644 --- a/src/quicer_nif.erl +++ b/src/quicer_nif.erl @@ -20,6 +20,8 @@ , reg_open/1 , reg_close/0 , listen/2 + , start_listener/3 + , stop_listener/1 , close_listener/1 , async_connect/3 , async_accept/2 @@ -113,10 +115,18 @@ reg_close() -> listen(_ListenOn, _Options) -> erlang:nif_error(nif_library_not_loaded). +-spec start_listener(listener_handle(), listen_on(), listen_opts()) -> ok | {error, closed | badarg}. +start_listener(_Listener, _ListenOn, _Opts) -> + erlang:nif_error(nif_library_not_loaded). + -spec close_listener(listener_handle()) -> ok | {error, closed | badarg}. close_listener(_Listener) -> erlang:nif_error(nif_library_not_loaded). +-spec stop_listener(listener_handle()) -> ok | {error, closed | badarg}. +stop_listener(_Listener) -> + erlang:nif_error(nif_library_not_loaded). + -spec open_connection() -> {ok, connection_handle()} | {error, atom_reason()}. open_connection() -> erlang:nif_error(nif_library_not_loaded). diff --git a/test/quicer_SUITE.erl b/test/quicer_SUITE.erl index e86a2183..c6730499 100644 --- a/test/quicer_SUITE.erl +++ b/test/quicer_SUITE.erl @@ -58,6 +58,8 @@ , tc_open_listener_inval_cacertfile_2/1 , tc_open_listener_inval_cacertfile_3/1 , tc_start_listener_alpn_too_long/1 + , tc_stop_start_listener/1 + , tc_stop_close_listener/1 , tc_close_listener/1 , tc_close_listener_twice/1 , tc_close_listener_dealloc/1 @@ -380,9 +382,9 @@ tc_open_listener_inval_cacertfile_1(Config) -> tc_open_listener_inval_cacertfile_2(Config) -> Port = select_port(), - ?assertMatch({ok, _}, - quicer:listen(Port, [ {cacertfile, [1,2,3,4]} - | default_listen_opts(Config)])), + {ok, L} = quicer:listen(Port, [ {cacertfile, [1,2,3,4]} + | default_listen_opts(Config)]), + ok = quicer:close_listener(L), ok. tc_open_listener_inval_cacertfile_3(Config) -> @@ -409,7 +411,8 @@ tc_open_listener_with_cert_password(Config) -> , {keyfile, filename:join(DataDir, "server-password.key")} , {password, ?SERVER_KEY_PASSWORD} ], - {ok, _L} = quicer:listen(Port, default_listen_opts(PasswordCerts ++ Config)), + {ok, L} = quicer:listen(Port, default_listen_opts(PasswordCerts ++ Config)), + quicer:close_listener(L), ok. tc_open_listener_with_wrong_cert_password(Config) -> @@ -473,23 +476,39 @@ tc_get_listener_opt_stats(Config) -> quicer:close_listener(L). tc_close_listener(_Config) -> - {error,badarg} = quicer:close_listener(make_ref()). + {error, badarg} = quicer:close_listener(make_ref()). tc_close_listener_twice(Config) -> Port = select_port(), {ok, L} = quicer:listen(Port, default_listen_opts(Config)), - quicer:close_listener(L), - quicer:close_listener(L). + ok = quicer:close_listener(L), + %% follow OTP behavior, already closed + ok = quicer:close_listener(L). tc_close_listener_dealloc(Config) -> Port = select_port(), {Pid, Ref} = spawn_monitor(fun() -> - {ok, _L} = quicer:listen(Port, default_listen_opts(Config)) + {ok, L} = quicer:listen(Port, default_listen_opts(Config)), + exit(L) end), - receive {'DOWN', Ref, process, Pid, normal} -> - ok + receive {'DOWN', Ref, process, Pid, L} -> + quicer:close_listener(L) end. +tc_stop_start_listener(Config) -> + Port = select_port(), + LConf = default_listen_opts(Config), + {ok, L} = quicer:listen(Port, LConf), + ok = quicer:stop_listen(L), + ok = quicer:start_listen(L, Port, LConf), + ok = quicer:close_listener(L). + +tc_stop_close_listener(Config) -> + Port = select_port(), + {ok, L} = quicer:listen(Port, default_listen_opts(Config)), + ok = quicer:stop_listen(L), + ok = quicer:close_listener(L, 0). + tc_start_listener_alpn_too_long(Config) -> Port = select_port(), {Pid, Ref} = @@ -506,7 +525,8 @@ tc_start_acceptor_without_callback(Config) -> Port = select_port(), {ok, L} = quicer:listen(Port, default_listen_opts(Config)), ?assertEqual({error, missing_conn_callback}, - quicer_connection:start_link(undefined, L, {[],[],[]}, self())). + quicer_connection:start_link(undefined, L, {[],[],[]}, self())), + quicer:close_listener(L). tc_get_listeners(Config) -> ListenerOpts = [{conn_acceptors, 32} | default_listen_opts(Config)], @@ -3408,6 +3428,7 @@ simple_conn_server(Owner, Config, Port) -> simple_conn_server_loop(L, Conn, Owner) -> receive done -> + quicer:close_connection(Conn), quicer:close_listener(L), ok; peercert ->