From 56e9e451176fa5a60eb4ddcfb59a5c0b49fa28f3 Mon Sep 17 00:00:00 2001 From: William Yang Date: Fri, 6 Oct 2023 16:01:28 +0200 Subject: [PATCH] refactor: closing msquic handles early, part 1 --- c_src/quicer_connection.c | 30 +++++++++++++++++++++++++++++- c_src/quicer_ctx.c | 2 +- c_src/quicer_listener.c | 6 ++++++ c_src/quicer_nif.c | 30 ++++++++++++++++++------------ c_src/quicer_stream.c | 21 +++++++++++++++++---- c_src/quicer_vsn.h.in | 2 ++ 6 files changed, 73 insertions(+), 18 deletions(-) diff --git a/c_src/quicer_connection.c b/c_src/quicer_connection.c index a3956397..1a80b3bd 100644 --- a/c_src/quicer_connection.c +++ b/c_src/quicer_connection.c @@ -376,6 +376,14 @@ _IRQL_requires_max_(DISPATCH_LEVEL) if (is_destroy) { + // MsQuic->SetCallbackHandler(Connection, NULL, NULL); + c_ctx->Connection = NULL; + MsQuic->ConnectionClose(Connection); + if (c_ctx->config_resource) + { + enif_release_resource(c_ctx->config_resource); + c_ctx->config_resource = NULL; + } destroy_c_ctx(c_ctx); } return status; @@ -485,6 +493,13 @@ ServerConnectionCallback(HQUIC Connection, if (is_destroy) { + c_ctx->Connection = NULL; + MsQuic->ConnectionClose(Connection); + if (c_ctx->config_resource) + { + enif_release_resource(c_ctx->config_resource); + c_ctx->config_resource = NULL; + } destroy_c_ctx(c_ctx); } @@ -806,7 +821,15 @@ async_connect3(ErlNifEnv *env, */ // c_ctx->is_closed = TRUE; - c_ctx->Connection = NULL; + if (Status != QUIC_STATUS_INVALID_PARAMETER) + { + c_ctx->Connection = NULL; + } + + if (c_ctx->config_resource) + { + destroy_config_ctx(c_ctx->config_resource); + } res = ERROR_TUPLE_2(ATOM_CONN_START_ERROR); TP_NIF_3(start_fail, (uintptr_t)(c_ctx->Connection), Status); @@ -1056,6 +1079,11 @@ continue_connection_handshake(QuicerConnCTX *c_ctx) return QUIC_STATUS_INTERNAL_ERROR; } + if (!c_ctx->Connection) + { + return QUIC_STATUS_INVALID_STATE; + } + if (QUIC_FAILED( Status = MsQuic->ConnectionSetConfiguration( c_ctx->Connection, c_ctx->config_resource->Configuration))) diff --git a/c_src/quicer_ctx.c b/c_src/quicer_ctx.c index db6ba7eb..a273af97 100644 --- a/c_src/quicer_ctx.c +++ b/c_src/quicer_ctx.c @@ -257,7 +257,7 @@ destroy_s_ctx(QuicerStreamCTX *s_ctx) enif_free_env(s_ctx->imm_env); // Since enif_release_resource is async call, // we should demon the owner now! - enif_demonitor_process(s_ctx->env, s_ctx, &s_ctx->owner_mon); + // enif_demonitor_process(s_ctx->env, s_ctx, &s_ctx->owner_mon); enif_release_resource(s_ctx); } diff --git a/c_src/quicer_listener.c b/c_src/quicer_listener.c index 2e1d1d2f..bf291f07 100644 --- a/c_src/quicer_listener.c +++ b/c_src/quicer_listener.c @@ -578,6 +578,7 @@ get_listenersX(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) QuicerRegistrationCTX *r_ctx = NULL; if (argc == 0) { + // @TODO lock? r_ctx = G_r_ctx; } else @@ -589,6 +590,11 @@ get_listenersX(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) } ERL_NIF_TERM res = enif_make_list(env, 0); + if (r_ctx == NULL) + { + return res; + } + enif_mutex_lock(r_ctx->lock); CXPLAT_LIST_ENTRY *Entry = r_ctx->Listeners.Flink; while (Entry != &r_ctx->Listeners) diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index c260154b..8b2dca67 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -804,10 +804,12 @@ resource_listener_down_callback(__unused_parm__ ErlNifEnv *env, { l_ctx->is_stopped = TRUE; // We only stop here, but not close it, because possible subsequent - // scenarios a. Some pid could still start the stopped listener with nif - // handle b. Some pid could still close the stopped listener with nif + // scenarios: + // a. Some pid could still start the stopped listener with nif // handle - // 3. We close it in resource_listener_dealloc_callback anyway. + // b. Some pid could still close the stopped listener with nif + // handle + // c. We close it in resource_listener_dealloc_callback anyway. MsQuic->ListenerStop(l_ctx->Listener); } enif_mutex_unlock(l_ctx->lock); @@ -837,6 +839,10 @@ resource_listener_dealloc_callback(__unused_parm__ ErlNifEnv *env, void *obj) // process is able to access the listener via any l_ctx MsQuic->ListenerClose(l_ctx->Listener); } + else + { + TP_CB_3(skip, (uintptr_t)l_ctx->Listener, 0); + } if (l_ctx->cacertfile) { @@ -1042,17 +1048,17 @@ on_load(ErlNifEnv *env, TP_NIF_3(start, &MsQuic, 0); if (!enif_get_uint(env, loadinfo, &load_vsn)) - { - load_vsn = 0; - } + { + load_vsn = 0; + } // This check avoid erlang module loaded // incompatible NIF library if (load_vsn != QUICER_ABI_VERSION) - { - TP_NIF_3(end, &MsQuic, 1); - return 1; // any value except 0 is error - } + { + TP_NIF_3(end, &MsQuic, 1); + return 1; // any value except 0 is error + } init_atoms(env); open_resources(env); @@ -1141,7 +1147,7 @@ static void on_unload(__unused_parm__ ErlNifEnv *env, __unused_parm__ void *priv_data) { // @TODO clean all the leakages before close the lib - //closeLib(env, 0, NULL); + // closeLib(env, 0, NULL); } static ERL_NIF_TERM @@ -1518,7 +1524,7 @@ static ErlNifFunc nif_funcs[] = { { "close_lib", 0, closeLib, 0 }, { "reg_open", 0, registration, 0 }, { "reg_open", 1, registration, 0 }, - { "reg_close", 0, deregistration, 0 }, + { "reg_close", 0, deregistration, 1 }, { "new_registration", 2, new_registration2, 0}, { "shutdown_registration", 1, shutdown_registration_x, 0}, { "shutdown_registration", 3, shutdown_registration_x, 0}, diff --git a/c_src/quicer_stream.c b/c_src/quicer_stream.c index 04e12f0f..8fe26538 100644 --- a/c_src/quicer_stream.c +++ b/c_src/quicer_stream.c @@ -130,12 +130,14 @@ ServerStreamCallback(HQUIC Stream, void *Context, QUIC_STREAM_EVENT *Event) if (is_destroy) { s_ctx->is_closed = TRUE; + s_ctx->Stream = NULL; } enif_mutex_unlock(s_ctx->lock); if (is_destroy) { + MsQuic->StreamClose(Stream); // must be called after mutex unlock destroy_s_ctx(s_ctx); } @@ -160,6 +162,12 @@ _IRQL_requires_max_(DISPATCH_LEVEL) switch (Event->Type) { case QUIC_STREAM_EVENT_START_COMPLETE: + + if (QUIC_STATUS_STREAM_LIMIT_REACHED == Event->START_COMPLETE.Status) + { + // @TODO check what to do now: maybe set is_destroy = TRUE; + } + status = handle_stream_event_start_complete(s_ctx, Event); break; @@ -228,6 +236,7 @@ _IRQL_requires_max_(DISPATCH_LEVEL) if (is_destroy) { s_ctx->is_closed = TRUE; + MsQuic->SetCallbackHandler(Stream, NULL, NULL); } enif_mutex_unlock(s_ctx->lock); @@ -235,6 +244,8 @@ _IRQL_requires_max_(DISPATCH_LEVEL) if (is_destroy) { // must be called after mutex unlock, + s_ctx->Stream = NULL; + MsQuic->StreamClose(Stream); destroy_s_ctx(s_ctx); } return status; @@ -341,6 +352,7 @@ async_start_stream2(ErlNifEnv *env, // Now we have Stream handle s_ctx->eHandle = enif_make_resource(s_ctx->imm_env, s_ctx); + res = enif_make_copy(env, s_ctx->eHandle); // // Starts the bidirectional stream. By default, the peer is not notified of @@ -352,9 +364,6 @@ async_start_stream2(ErlNifEnv *env, // destroy_s_ctx should not be called here return ERROR_TUPLE_3(ATOM_STREAM_START_ERROR, ATOM_STATUS(Status)); } - - res = enif_make_copy(env, s_ctx->eHandle); - // NOTE: Set is_closed to FALSE (s_ctx->is_closed = FALSE;) // must be done in the worker callback (for // QUICER_STREAM_EVENT_MASK_START_COMPLETE) to avoid race cond. @@ -664,6 +673,11 @@ send3(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) } enif_mutex_lock(s_ctx->lock); + if (!s_ctx->Stream) + { + res = ERROR_TUPLE_2(ATOM_CLOSED); + goto ErrorExit; + } send_ctx->s_ctx = s_ctx; @@ -1000,7 +1014,6 @@ handle_stream_event_start_complete(QuicerStreamCTX *s_ctx, assert(env); assert(QUIC_STREAM_EVENT_START_COMPLETE == Event->Type); // Only for Local initiated stream - s_ctx->is_closed = FALSE; if (s_ctx->event_mask & QUICER_STREAM_EVENT_MASK_START_COMPLETE) { ERL_NIF_TERM props_name[] diff --git a/c_src/quicer_vsn.h.in b/c_src/quicer_vsn.h.in index 7f9cd1f7..acc3a6a9 100644 --- a/c_src/quicer_vsn.h.in +++ b/c_src/quicer_vsn.h.in @@ -1,6 +1,8 @@ #ifndef __QUICER_VSN_H_ #define __QUICER_VSN_H_ +// clang-format off #define QUICER_ABI_VERSION @QUICER_ABI_VERSION@ +// clang-format on #endif //__QUICER_VSN_H_