Skip to content

Commit

Permalink
refactor: closing msquic handles early, part 1
Browse files Browse the repository at this point in the history
  • Loading branch information
qzhuyan committed Oct 6, 2023
1 parent 8bfb322 commit 390315e
Show file tree
Hide file tree
Showing 6 changed files with 73 additions and 18 deletions.
30 changes: 29 additions & 1 deletion c_src/quicer_connection.c
Original file line number Diff line number Diff line change
Expand Up @@ -376,6 +376,14 @@ _IRQL_requires_max_(DISPATCH_LEVEL)

if (is_destroy)
{
// MsQuic->SetCallbackHandler(Connection, NULL, NULL);

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
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;
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)))
Expand Down
2 changes: 1 addition & 1 deletion c_src/quicer_ctx.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
6 changes: 6 additions & 0 deletions c_src/quicer_listener.c
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
Expand Down
30 changes: 18 additions & 12 deletions c_src/quicer_nif.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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},
Expand Down
21 changes: 17 additions & 4 deletions c_src/quicer_stream.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand All @@ -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;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
}

Check notice

Code scanning / CodeQL

Futile conditional Note

If-statement with an empty then-branch and no else-branch.

status = handle_stream_event_start_complete(s_ctx, Event);
break;

Expand Down Expand Up @@ -228,13 +236,16 @@ _IRQL_requires_max_(DISPATCH_LEVEL)
if (is_destroy)
{
s_ctx->is_closed = TRUE;
MsQuic->SetCallbackHandler(Stream, NULL, NULL);
}

enif_mutex_unlock(s_ctx->lock);

if (is_destroy)
{
// must be called after mutex unlock,
s_ctx->Stream = NULL;
MsQuic->StreamClose(Stream);
destroy_s_ctx(s_ctx);
}
return status;
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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[]
Expand Down
2 changes: 2 additions & 0 deletions c_src/quicer_vsn.h.in
Original file line number Diff line number Diff line change
@@ -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_

0 comments on commit 390315e

Please sign in to comment.