From abd45e4ac65dbe6f5be7b84b545af2fe2a490f67 Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 3 Oct 2023 13:23:02 +0200 Subject: [PATCH 01/22] doc: nif reloading --- c_src/quicer_nif.c | 73 +++++++++++++++++++++++++++++++++++++++++----- src/quicer_nif.erl | 1 + 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index 90478e3e..70177c5b 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -963,7 +963,7 @@ resource_reg_dealloc_callback(__unused_parm__ ErlNifEnv *env, void *obj) /* ** on_load is called when the NIF library is loaded and no previously loaded -*library exists for this module. +* library exists for this module. */ static int on_load(ErlNifEnv *env, @@ -1034,9 +1034,19 @@ on_load(ErlNifEnv *env, } /* -** on_upgrade is called when the NIF library is loaded and there is old code of -*this module with a loaded NIF library. -*/ + * on_upgrade is called when the NIF library is loaded and there is old code of + * this module with a loaded NIF library. + * + * But new code could be the same as old code, that is, the same msquic + * library is mapped into process memory. To distinguish the two cases, the + * `MsQuic` API handle is checked since it is init as NULL for new loading. If + * MsQuic is NULL, then it is a new load, that two msquic libraries (new and + * old) are mapped into process memory, If MsQuic is not NULL, then it is + * already initilized and there is still one msquic library in process memory. + * + * In any case above, we return success. + */ + static int on_upgrade(ErlNifEnv *env, void **priv_data, @@ -1047,9 +1057,58 @@ on_upgrade(ErlNifEnv *env, } /* -** unload is called when the module code that the NIF library belongs to is -*purged as old. New code of the same module may or may not exist. -*/ +** on_unload is called when the module code that the NIF library belongs to is +* purged as old. +* +* New code of the same module may or may not exist. +* +* But there are three cases: +* +* Case A: No new code of the same module exists. +* arg `priv_data` is not NULL. +* +* It is ok to teardown the MsQuic with API handle and then close the +* API handle. +* +* Case B: New code of the same module exists and it uses the same NIF DSO. +* arg `priv_data` is NULL. +* +* It could be checked with `quicer:nif_mapped()` +* +* It is *NOT* ok to teardown the MsQuic since the new code is +* still using it. +* +* Case C: New code of the same module exists and it uses different NIF DSO. +* arg `priv_data` is not NULL. +* AND +* &MsQuic != the 'lib_api_ptr' in priv_data +* +* This could be checked with `quicer:nif_mapped()` +* +* It is ok to teardown the MsQuic with API handle and then close the +* API handle. + +* +* @NOTE 1. This callback will *NOT* be called when the module is purged +* while there are opening resources. +* When new code of the same module exists, the resources will be +* taken over by the new code thus it will get called for the +* old code. +* +* @NOTE 2. The `MsQuic` and `GRegistration` are in library scope. +* +* @NOTE 3: It is very important to shutdown all the MsQuic Registrations +* before return to avoid unexpected behaviour after NIF DSO is +* unmapped by OS. +* +* @NOTE 4: For safty, it is ok to dlopen the shared library by calling +* quicer:dlopen/1, so we will have a refcnt on it and it won't +* be unmapped by OS. +* +* @NOTE 5: 'same NIF DSO' means same shared library file that is managed +* by OS. +* Two copies of the same shared library in OS are different NIF DSOs. +* */ static void on_unload(__unused_parm__ ErlNifEnv *env, __unused_parm__ void *priv_data) { diff --git a/src/quicer_nif.erl b/src/quicer_nif.erl index 73e0a628..d50f1e71 100644 --- a/src/quicer_nif.erl +++ b/src/quicer_nif.erl @@ -56,6 +56,7 @@ , get_listeners/1 ]). +%% @NOTE: In embedded mode, first all modules are loaded. Then all on_load functions are called. -on_load(init/0). -include_lib("kernel/include/file.hrl"). From 6cdc549b7765ef6911d986a27c3c8fc17087f0bc Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 3 Oct 2023 16:32:59 +0200 Subject: [PATCH 02/22] build: DSO with versioning suffix --- CMakeLists.txt | 19 ++++++++++++++----- Makefile | 3 +++ build.sh | 1 + src/quicer_nif.erl | 2 +- 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index bcd215cc..a49cfbd2 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -5,6 +5,10 @@ project(quicer) SET(Erlang_EI_INCLUDE_DIRS ${Erlang_OTP_LIB_DIR}/${Erlang_EI_DIR}/include) SET(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/priv/) +if(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT) + set(CMAKE_INSTALL_PREFIX ${PROJECT_SOURCE_DIR}/priv/ CACHE PATH "..." FORCE) +endif() + # For cerl picking up the OTP_ROOT if (DEFINED ENV{Erlang_OTP_ROOT_DIR}) SET(Erlang_OTP_ROOT_DIR $ENV{Erlang_OTP_ROOT_DIR}) @@ -15,6 +19,12 @@ EXECUTE_PROCESS( ) endif() +if (DEFINED ENV{QUICER_VERSION}) + set(QUICER_VERSION $ENV{QUICER_VERSION}) +else() + set(QUICER_VERSION "0") +endif() + if (DEFINED ENV{CMAKE_BUILD_TYPE}) set(CMAKE_BUILD_TYPE $ENV{CMAKE_BUILD_TYPE}) else() @@ -138,10 +148,9 @@ add_dependencies(quicer_static msquic) set_target_properties(quicer_nif PROPERTIES - LIBRARY_OUTPUT_DIRECTORY ${PROJECT_SOURCE_DIR}/priv/ + LIBRARY_OUTPUT_NAME quicer_nif + VERSION ${QUICER_VERSION} ) +include(GNUInstallDirs) +install(TARGETS quicer_nif LIBRARY DESTINATION ${PROJECT_SOURCE_DIR}/priv/) -if (QUIC_ENABLE_LOGGING STREQUAL "ON" AND QUIC_LOGGING_TYPE STREQUAL "lttng") - set_target_properties(msquic.lttng PROPERTIES LIBRARY_OUTPUT_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/priv) - set_target_properties(msquic.lttng PROPERTIES LIBRARY_OUTPUT_DIRECTORY_DEBUG ${CMAKE_CURRENT_SOURCE_DIR}/priv) -endif() diff --git a/Makefile b/Makefile index 9a323c91..576eeb4e 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,8 @@ REBAR := rebar3 +QUICER_VERSION ?= $(shell git describe --tags --always) +export QUICER_VERSION + .PHONY: all all: compile diff --git a/build.sh b/build.sh index 65d4fa4d..8ab89935 100755 --- a/build.sh +++ b/build.sh @@ -17,6 +17,7 @@ build() { ./get-msquic.sh "$MSQUIC_VERSION" cmake -B c_build make -j "$JOBS" -C c_build + make install -C c_build ## MacOS if [ -f priv/libquicer_nif.dylib ]; then # https://developer.apple.com/forums/thread/696460 diff --git a/src/quicer_nif.erl b/src/quicer_nif.erl index d50f1e71..6e905b31 100644 --- a/src/quicer_nif.erl +++ b/src/quicer_nif.erl @@ -89,7 +89,7 @@ init() -> {ok, debug} | %% opened with lttng debug library loaded (if present) {error, open_failed, atom_reason()}. open_lib() -> - LibFile = case locate_lib(priv_dir(), "libmsquic.lttng.so") of + LibFile = case locate_lib(priv_dir(), "lib/libmsquic.lttng.so") of {ok, File} -> File; {error, _} -> From dc497e032321957ebb80e9260e909935d65d5e8a Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 3 Oct 2023 19:30:06 +0200 Subject: [PATCH 03/22] refactor: quicer_nif.c on_load --- c_src/quicer_nif.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index 70177c5b..54125ef0 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -961,25 +961,21 @@ resource_reg_dealloc_callback(__unused_parm__ ErlNifEnv *env, void *obj) TP_CB_3(end, (uintptr_t)obj, 0); } -/* -** on_load is called when the NIF library is loaded and no previously loaded -* library exists for this module. -*/ -static int -on_load(ErlNifEnv *env, - __unused_parm__ void **priv_data, - __unused_parm__ ERL_NIF_TERM loadinfo) +static void +init_atoms(ErlNifEnv *env) { - int ret_val = 0; - -// init atoms in use. + // init atoms in use. #define ATOM(name, val) \ { \ (name) = enif_make_atom(env, #val); \ } INIT_ATOMS #undef ATOM +} +static void +open_resources(ErlNifEnv *env) +{ ErlNifResourceFlags flags = (ErlNifResourceFlags)(ERL_NIF_RT_CREATE | ERL_NIF_RT_TAKEOVER); @@ -1029,6 +1025,22 @@ on_load(ErlNifEnv *env, &streamInit, // init callbacks flags, NULL); +} + +/* +** on_load is called when the NIF library is loaded and no previously loaded +* library exists for this module. +*/ +static int +on_load(ErlNifEnv *env, + __unused_parm__ void **priv_data, + __unused_parm__ ERL_NIF_TERM loadinfo) +{ + int ret_val = 0; + + init_atoms(env); + + open_resources(env); return ret_val; } From ff4b802416304094741ca989945ad263ef10ab35 Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 3 Oct 2023 20:45:47 +0200 Subject: [PATCH 04/22] feat: abi version --- CMakeLists.txt | 19 +++++++++++++++++-- c_src/quicer_nif.c | 15 ++++++++++++++- c_src/quicer_vsn.h.in | 6 ++++++ include/quicer_vsn.hrl.in | 4 ++++ src/quicer_nif.erl | 9 ++++++++- 5 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 c_src/quicer_vsn.h.in create mode 100644 include/quicer_vsn.hrl.in diff --git a/CMakeLists.txt b/CMakeLists.txt index a49cfbd2..ffc07bad 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -2,6 +2,10 @@ cmake_minimum_required(VERSION 3.16) project(quicer) +## NIF library ABI version +## Bump manually when ABI changes +set(QUICER_ABI_VERSION 1) + SET(Erlang_EI_INCLUDE_DIRS ${Erlang_OTP_LIB_DIR}/${Erlang_EI_DIR}/include) SET(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${PROJECT_BINARY_DIR}/priv/) @@ -74,6 +78,16 @@ set(QUIC_BUILD_PERF "OFF") set(QUIC_TLS_SECRETS_SUPPORT "ON") +configure_file( + ${CMAKE_CURRENT_SOURCE_DIR}/include/quicer_vsn.hrl.in + ${CMAKE_CURRENT_SOURCE_DIR}/include/quicer_vsn.hrl +) + +configure_file( + ${CMAKE_CURRENT_SOURCE_DIR}/c_src/quicer_vsn.h.in + ${CMAKE_CURRENT_BINARY_DIR}/c_src/quicer_vsn.h +) + # src files set(SOURCES c_src/quicer_nif.c @@ -106,7 +120,8 @@ add_compile_options(-DSO_ATTACH_REUSEPORT_CBPF=51) # for lttng, quicer_tp.h include_directories(c_src) - +# for templ files +include_directories(${CMAKE_CURRENT_BINARY_DIR}/c_src/) add_subdirectory(msquic) add_library(quicer_static STATIC ${SOURCES}) @@ -149,7 +164,7 @@ add_dependencies(quicer_static msquic) set_target_properties(quicer_nif PROPERTIES LIBRARY_OUTPUT_NAME quicer_nif - VERSION ${QUICER_VERSION} + VERSION ${QUICER_ABI_VERSION}-${QUICER_VERSION} ) include(GNUInstallDirs) install(TARGETS quicer_nif LIBRARY DESTINATION ${PROJECT_SOURCE_DIR}/priv/) diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index 54125ef0..debc91dd 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -19,6 +19,7 @@ limitations under the License. #include #include "quicer_listener.h" +#include "quicer_vsn.h" #include #include @@ -1034,10 +1035,22 @@ open_resources(ErlNifEnv *env) static int on_load(ErlNifEnv *env, __unused_parm__ void **priv_data, - __unused_parm__ ERL_NIF_TERM loadinfo) + ERL_NIF_TERM loadinfo) { int ret_val = 0; + unsigned load_vsn = 0; + + if (!enif_get_uint(env, loadinfo, &load_vsn)) + { + load_vsn = 0; + } + + if (load_vsn != QUICER_ABI_VERSION) + { + return -1; + } + init_atoms(env); open_resources(env); diff --git a/c_src/quicer_vsn.h.in b/c_src/quicer_vsn.h.in new file mode 100644 index 00000000..7f9cd1f7 --- /dev/null +++ b/c_src/quicer_vsn.h.in @@ -0,0 +1,6 @@ +#ifndef __QUICER_VSN_H_ +#define __QUICER_VSN_H_ + +#define QUICER_ABI_VERSION @QUICER_ABI_VERSION@ + +#endif //__QUICER_VSN_H_ diff --git a/include/quicer_vsn.hrl.in b/include/quicer_vsn.hrl.in new file mode 100644 index 00000000..02d0b4e1 --- /dev/null +++ b/include/quicer_vsn.hrl.in @@ -0,0 +1,4 @@ +-ifndef(QUICER_VSN_HRL). +-define(QUICER_VSN_HRL, true). +-define(QUICER_ABI_VERSION, @QUICER_ABI_VERSION@). +-endif. diff --git a/src/quicer_nif.erl b/src/quicer_nif.erl index 6e905b31..c2fb9b5a 100644 --- a/src/quicer_nif.erl +++ b/src/quicer_nif.erl @@ -56,18 +56,25 @@ , get_listeners/1 ]). +-export([abi_version/0]). + %% @NOTE: In embedded mode, first all modules are loaded. Then all on_load functions are called. -on_load(init/0). -include_lib("kernel/include/file.hrl"). -include("quicer.hrl"). -include("quicer_types.hrl"). +-include("quicer_vsn.hrl"). + +-spec abi_version() -> integer(). +abi_version() -> + ?QUICER_ABI_VERSION. -spec init() -> ok. init() -> NifName = "libquicer_nif", {ok, Niflib} = locate_lib(priv_dir(), NifName), - ok = erlang:load_nif(Niflib, 0), + ok = erlang:load_nif(Niflib, ?QUICER_ABI_VERSION), %% It could cause segfault if MsQuic library is not opened nor registered. %% here we have added dummy calls, and it should cover most of cases %% unless caller wants to call erlang:load_nif/1 and then call quicer_nif From 84a9cef813b4106e5350a0c0ee0c4ca05a6a91f6 Mon Sep 17 00:00:00 2001 From: William Yang Date: Wed, 4 Oct 2023 09:42:08 +0200 Subject: [PATCH 05/22] test: abi version --- src/quicer.erl | 8 ++++++++ test/quicer_SUITE.erl | 6 ++++++ 2 files changed, 14 insertions(+) diff --git a/src/quicer.erl b/src/quicer.erl index b067ba3a..655b1fdc 100644 --- a/src/quicer.erl +++ b/src/quicer.erl @@ -120,9 +120,17 @@ , terminate_listener/1 ]). +%% versions +-export([abi_version/0]). + -type connection_opts() :: proplists:proplist() | quicer_connection:opts(). -type listener_opts() :: proplists:proplist() | quicer_listener:listener_opts(). +%% @doc Return ABI version of the library. +-spec abi_version() -> quicer_nif:abi_version(). +abi_version() -> + quicer_nif:abi_version(). + %% @doc Quicer library must be opened before any use. %% %% This is called automatically while quicer application is started diff --git a/test/quicer_SUITE.erl b/test/quicer_SUITE.erl index 49b5475a..c0f1821a 100644 --- a/test/quicer_SUITE.erl +++ b/test/quicer_SUITE.erl @@ -144,6 +144,9 @@ , tc_peercert_client_nocert/1 , tc_peercert_server/1 , tc_peercert_server_nocert/1 + + %% Versions test + , tc_abi_version/1 %% testcase to verify env works %% , tc_network/1 ]). @@ -2496,6 +2499,9 @@ tc_peercert_server_nocert(Config) -> ensure_server_exit_normal(Ref), ok. +tc_abi_version(Config) -> + ?assertEqual(1, quicer:abi_version()). + %%% ==================== %%% Internal helpers %%% ==================== From 9640671140b01a7554e28f24ad0382ad45a45370 Mon Sep 17 00:00:00 2001 From: William Yang Date: Wed, 4 Oct 2023 13:07:36 +0200 Subject: [PATCH 06/22] test: add upgrade test suite --- c_src/quicer_nif.c | 12 +- src/quicer_nif.erl | 47 ++++--- test/quicer_upgrade_SUITE.erl | 236 ++++++++++++++++++++++++++++++++++ 3 files changed, 276 insertions(+), 19 deletions(-) create mode 100644 test/quicer_upgrade_SUITE.erl diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index debc91dd..c260154b 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -1038,23 +1038,26 @@ on_load(ErlNifEnv *env, ERL_NIF_TERM loadinfo) { int ret_val = 0; - unsigned load_vsn = 0; + TP_NIF_3(start, &MsQuic, 0); if (!enif_get_uint(env, loadinfo, &load_vsn)) { load_vsn = 0; } + // This check avoid erlang module loaded + // incompatible NIF library if (load_vsn != QUICER_ABI_VERSION) { - return -1; + TP_NIF_3(end, &MsQuic, 1); + return 1; // any value except 0 is error } init_atoms(env); - open_resources(env); + TP_NIF_3(end, &MsQuic, 0); return ret_val; } @@ -1137,7 +1140,8 @@ on_upgrade(ErlNifEnv *env, static void on_unload(__unused_parm__ ErlNifEnv *env, __unused_parm__ void *priv_data) { - closeLib(env, 0, NULL); + // @TODO clean all the leakages before close the lib + //closeLib(env, 0, NULL); } static ERL_NIF_TERM diff --git a/src/quicer_nif.erl b/src/quicer_nif.erl index c2fb9b5a..451b0caf 100644 --- a/src/quicer_nif.erl +++ b/src/quicer_nif.erl @@ -58,6 +58,9 @@ -export([abi_version/0]). +%% for test +-export([init/1]). + %% @NOTE: In embedded mode, first all modules are loaded. Then all on_load functions are called. -on_load(init/0). @@ -65,6 +68,7 @@ -include("quicer.hrl"). -include("quicer_types.hrl"). -include("quicer_vsn.hrl"). +-include_lib("snabbkaffe/include/snabbkaffe.hrl"). -spec abi_version() -> integer(). abi_version() -> @@ -72,24 +76,37 @@ abi_version() -> -spec init() -> ok. init() -> + ABIVsn = case persistent_term:get({'_quicer_overrides_', abi_version}, undefined) of + undefined -> abi_version(); + Vsn -> Vsn + end, + init(ABIVsn). + +init(ABIVsn) -> NifName = "libquicer_nif", {ok, Niflib} = locate_lib(priv_dir(), NifName), - ok = erlang:load_nif(Niflib, ?QUICER_ABI_VERSION), - %% It could cause segfault if MsQuic library is not opened nor registered. - %% here we have added dummy calls, and it should cover most of cases - %% unless caller wants to call erlang:load_nif/1 and then call quicer_nif - %% without opened library to suicide. - %% - %% Note, we could do same dummy calls in nif instead but it might mess up the reference counts. - {ok, _} = open_lib(), - %% dummy reg open - case reg_open() of - ok -> ok; - {error, badarg} -> - %% already opened - ok + case erlang:load_nif(Niflib, ABIVsn) of + ok -> + %% It could cause segfault if MsQuic library is not opened nor registered. + %% here we have added dummy calls, and it should cover most of cases + %% unless caller wants to call erlang:load_nif/1 and then call quicer_nif + %% without opened library to suicide. + %% + %% Note, we could do same dummy calls in nif instead but it might mess up the reference counts. + {ok, _} = open_lib(), + %% dummy reg open + case reg_open() of + ok -> ok; + {error, badarg} -> + %% already opened + ok + end; + {error, _Reason} = Res-> + %% load fail, but beam will keep using current vsn if presents. + ?tp_ignore_side_effects_in_prod(debug, + #{module => ?MODULE, event => init, result => Res}), + Res end. - -spec open_lib() -> {ok, true} | %% opened {ok, false} | %% already opened diff --git a/test/quicer_upgrade_SUITE.erl b/test/quicer_upgrade_SUITE.erl new file mode 100644 index 00000000..78028f36 --- /dev/null +++ b/test/quicer_upgrade_SUITE.erl @@ -0,0 +1,236 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- +-module(quicer_upgrade_SUITE). + +-compile(export_all). + +-include_lib("common_test/include/ct.hrl"). +-include_lib("eunit/include/eunit.hrl"). +%%-------------------------------------------------------------------- +%% @spec suite() -> Info +%% Info = [tuple()] +%% @end +%%-------------------------------------------------------------------- +suite() -> + [{timetrap,{seconds,30}}]. + +%%-------------------------------------------------------------------- +%% @spec init_per_suite(Config0) -> +%% Config1 | {skip,Reason} | {skip_and_save,Reason,Config1} +%% Config0 = Config1 = [tuple()] +%% Reason = term() +%% @end +%%-------------------------------------------------------------------- +init_per_suite(Config) -> + %% close all listeners under global registration + [quicer:close_listener(L, 1000) || L <- quicer:get_listeners()], + Config. + +%%-------------------------------------------------------------------- +%% @spec end_per_suite(Config0) -> term() | {save_config,Config1} +%% Config0 = Config1 = [tuple()] +%% @end +%%-------------------------------------------------------------------- +end_per_suite(_Config) -> + ok. + +%%-------------------------------------------------------------------- +%% @spec init_per_group(GroupName, Config0) -> +%% Config1 | {skip,Reason} | {skip_and_save,Reason,Config1} +%% GroupName = atom() +%% Config0 = Config1 = [tuple()] +%% Reason = term() +%% @end +%%-------------------------------------------------------------------- +init_per_group(_GroupName, Config) -> + Config. + +%%-------------------------------------------------------------------- +%% @spec end_per_group(GroupName, Config0) -> +%% term() | {save_config,Config1} +%% GroupName = atom() +%% Config0 = Config1 = [tuple()] +%% @end +%%-------------------------------------------------------------------- +end_per_group(_GroupName, _Config) -> + ok. + +%%-------------------------------------------------------------------- +%% @spec init_per_testcase(TestCase, Config0) -> +%% Config1 | {skip,Reason} | {skip_and_save,Reason,Config1} +%% TestCase = atom() +%% Config0 = Config1 = [tuple()] +%% Reason = term() +%% @end +%%-------------------------------------------------------------------- +init_per_testcase(TestCase, Config) -> + case erlang:function_exported(?MODULE, TestCase, 2) of + false -> + Config; + true -> + ?MODULE:TestCase(init, Config) + end. + +%%-------------------------------------------------------------------- +%% @spec end_per_testcase(TestCase, Config0) -> +%% term() | {save_config,Config1} | {fail,Reason} +%% TestCase = atom() +%% Config0 = Config1 = [tuple()] +%% Reason = term() +%% @end +%%-------------------------------------------------------------------- +end_per_testcase(_TestCase, _Config) -> + ok. + +%%-------------------------------------------------------------------- +%% @spec groups() -> [Group] +%% Group = {GroupName,Properties,GroupsAndTestCases} +%% GroupName = atom() +%% Properties = [parallel | sequence | Shuffle | {RepeatType,N}] +%% GroupsAndTestCases = [Group | {group,GroupName} | TestCase] +%% TestCase = atom() +%% Shuffle = shuffle | {shuffle,{integer(),integer(),integer()}} +%% RepeatType = repeat | repeat_until_all_ok | repeat_until_all_fail | +%% repeat_until_any_ok | repeat_until_any_fail +%% N = integer() | forever +%% @end +%%-------------------------------------------------------------------- +groups() -> + []. + +%%-------------------------------------------------------------------- +%% @spec all() -> GroupsAndTestCases | {skip,Reason} +%% GroupsAndTestCases = [{group,GroupName} | TestCase] +%% GroupName = atom() +%% TestCase = atom() +%% Reason = term() +%% @end +%%-------------------------------------------------------------------- +all() -> + quicer_test_lib:all_tcs(?MODULE). + +%%-------------------------------------------------------------------- +%% @spec TestCase(Config0) -> +%% ok | exit() | {skip,Reason} | {comment,Comment} | +%% {save_config,Config1} | {skip_and_save,Reason,Config1} +%% Config0 = Config1 = [tuple()] +%% Reason = term() +%% Comment = term() +%% @end +%%-------------------------------------------------------------------- +tc_nif_module_is_loaded(_Config) -> + ?assertMatch({file, _}, code:is_loaded(quicer_nif)). + +tc_nif_module_purge(init, Config) -> + %% When we have both old code and new code + case code:load_file(quicer_nif) of + {module, quicer_nif} -> + ok; + {error, not_purged} -> + ok + end, + Config. +tc_nif_module_purge(_Config) -> + %% Then purge old code should success (test nif lib no crash) + ?assertEqual(false, code:purge(quicer_nif)). + +tc_nif_no_old_module_purge(init, Config) -> + %% Give there is no old code of quicer_nif present + code:purge(quicer_nif), + Config. +tc_nif_no_old_module_purge(_Config) -> + %% Then purge non existing old code should success (test nif lib no crash) + ?assertEqual(false, code:purge(quicer_nif)). + +tc_nif_module_reload(init, Config) -> + %% Given no old code of quicer_nif present + code:purge(quicer_nif), + Config. +tc_nif_module_reload(_Config) -> + %% When reload quicer_nif with same module file + %% Then load success + ?assertEqual({module, quicer_nif}, code:load_file(quicer_nif)). + +tc_nif_module_load_current(init, Config) -> + %% Given no new/old code of quicer_nif present + ensure_module_no_code(quicer_nif), + Config. +tc_nif_module_load_current(_Config) -> + %% When load quicer_nif, it should success + ?assertEqual({module, quicer_nif}, code:load_file(quicer_nif)). + +tc_nif_module_softpurge(init, Config) -> + %% When we have both old code and new code + case code:load_file(quicer_nif) of + {module, quicer_nif} -> + ok; + {error, not_purged} -> + ok + end, + Config. +tc_nif_module_softpurge(_Config) -> + ?assertEqual(true, code:soft_purge(quicer_nif)). + +tc_nif_module_no_reinit(_Config) -> + %% Given quicer_nif is not loaded + ensure_module_no_code(quicer_nif), + Res = {error, {reload, "NIF library already loaded (reload disallowed since OTP 20)."}}, + %% When calling quicer_nif:init/1 with ABI version 1 + %% Then it should still fail + ?assertEqual(Res, quicer_nif:init(1)). + +tc_nif_module_load_fail_dueto_mismatch_abiversion(init, Config) -> + %% Given quicer_nif is not loaded + ensure_module_no_code(quicer_nif), + %% When _quicer_overrides_ provides ABI version 0 which is reserved + persistent_term:put({'_quicer_overrides_', abi_version}, 0), + Config. +tc_nif_module_load_fail_dueto_mismatch_abiversion(_Config) -> + %% Then load quicer_nif should fail due to abi version mismatch + ?assertEqual({error, on_load_failure}, code:load_file(quicer_nif)), + persistent_term:erase({'_quicer_overrides_', abi_version}). + +tc_nif_module_upgrade_fail_dueto_mismatch_abiversion(init, Config) -> + %% Given quicer_nif has current code + ensure_module_current_vsn(quicer_nif), + %% When _quicer_overrides_ provides ABI version 0 which is reserved + persistent_term:put({'_quicer_overrides_', abi_version}, 0), + Config. +tc_nif_module_upgrade_fail_dueto_mismatch_abiversion(_Config) -> + %% Then upgrade quicer_nif should fail due to abi version mismatch + ?assertEqual({error, on_load_failure}, code:load_file(quicer_nif)), + persistent_term:erase({'_quicer_overrides_', abi_version}), + %% Then quicer_nif should still be loaded + ?assertMatch({file, _}, code:is_loaded(quicer_nif)). + +%% Helpers + +%% @doc ensure neither old nor new code is present +ensure_module_no_code(Module) -> + %% purge old if any + code:purge(Module), + %% current become old + code:delete(Module), + %% assert no current. + not_loaded = code:module_status(Module), + %% force purge this old + _ = code:purge(Module). + +%% @doc ensure only current code is present +ensure_module_current_vsn(Module) -> + ensure_module_no_code(Module), + _ = code:load_file(Module), + ok. From d48d70eedd8877d1862169fcd1b85c8b1672f9ef Mon Sep 17 00:00:00 2001 From: William Yang Date: Fri, 6 Oct 2023 11:56:49 +0200 Subject: [PATCH 07/22] test: ensure resource cleannup --- test/example_server_stream.erl | 5 +- test/quicer_SUITE.erl | 115 ++++++++++++++------ test/quicer_connection_SUITE.erl | 38 +++++-- test/quicer_echo_server_stream_callback.erl | 18 ++- test/quicer_listener_SUITE.erl | 7 +- test/quicer_reg_SUITE.erl | 5 +- test/quicer_snb_SUITE.erl | 37 ++++--- test/quicer_test_lib.erl | 44 +++++++- test/quicer_upgrade_SUITE.erl | 1 + 9 files changed, 202 insertions(+), 68 deletions(-) diff --git a/test/example_server_stream.erl b/test/example_server_stream.erl index 33276f03..ef168ffc 100644 --- a/test/example_server_stream.erl +++ b/test/example_server_stream.erl @@ -82,7 +82,10 @@ peer_send_aborted(Stream, ErrorCode, #{is_unidir := true, is_local := false} = S {ok, S}. peer_send_shutdown(Stream, _Flags, S) -> - ok = quicer:async_shutdown_stream(Stream, ?QUIC_STREAM_SHUTDOWN_FLAG_GRACEFUL, 0), + case quicer:async_shutdown_stream(Stream, ?QUIC_STREAM_SHUTDOWN_FLAG_GRACEFUL, 0) of + ok -> ok; + {error, _} -> ok + end, {ok, S}. send_complete(_Stream, false, S) -> diff --git a/test/quicer_SUITE.erl b/test/quicer_SUITE.erl index c0f1821a..98c2fe29 100644 --- a/test/quicer_SUITE.erl +++ b/test/quicer_SUITE.erl @@ -195,7 +195,10 @@ init_per_suite(Config) -> Config. end_per_suite(_Config) -> + quicer_test_lib:report_active_connections(), application:stop(quicer), + code:purge(quicer_nif), + code:delete(quicer_nif), ok. @@ -216,6 +219,7 @@ end_per_group(_Groupname, _Config) -> %%% Testcase specific setup/teardown %%%=================================================================== init_per_testcase(_TestCase, Config) -> + quicer_test_lib:cleanup_msquic(), [{timetrap, 5000} | Config]. end_per_testcase(tc_close_lib_test, _Config) -> @@ -231,10 +235,13 @@ end_per_testcase(tc_lib_re_registration_neg, _Config) -> end_per_testcase(tc_open_listener_neg_1, _Config) -> quicer:open_lib(), quicer:reg_open(); +end_per_testcase(tc_lib_registration_neg, _Config) -> + quicer:reg_open(); end_per_testcase(_TestCase, _Config) -> quicer:terminate_listener(mqtt), - Unhandled = quicer_test_lib:receive_all(), - Unhandled =/= [] andalso ct:comment("What left in the message queue: ~p", [Unhandled]), + quicer_test_lib:report_unhandled_messages(), + quicer_test_lib:report_active_connections(fun ct:pal/2), + ct:pal("Counters ~p", [quicer:perf_counters()]), ok. %%%=================================================================== @@ -324,7 +331,6 @@ tc_open_listener_inval_reg(Config) -> quicer:reg_open(), ok. - tc_stream_client_init(Config) -> Port = select_port(), Owner = self(), @@ -343,7 +349,6 @@ tc_stream_client_init(Config) -> ct:fail("timeout") end. - tc_stream_client_send_binary(Config) -> Port = select_port(), Owner = self(), @@ -701,7 +706,12 @@ tc_stream_send_after_conn_close(Config) -> %% a) Just close connection, stream is not created in QUIC %% b) Close the connection after the stream is created in QUIC ok = quicer:close_connection(Conn), - {error, stm_send_error, aborted} = quicer:send(Stm, <<"ping2">>), + case quicer:send(Stm, <<"ping2">>) of + {error, closed} -> + ok; + {error, stm_send_error, aborted} -> + ok + end, SPid ! done, ok = ensure_server_exit_normal(Ref) after 1000 -> @@ -921,7 +931,7 @@ tc_getopt(Config) -> receive {quic, <<"ping">>, Stm, _} -> ok end, ok = quicer:close_connection(Conn), %% @todo unsupp in msquic, leave it for now - {error, not_found} = quicer:getopt(Conn, param_conn_close_reason_phrase), + {error, _} = quicer:getopt(Conn, param_conn_close_reason_phrase), SPid ! done, ensure_server_exit_normal(Ref) after 5000 -> @@ -1012,9 +1022,13 @@ tc_get_stream_0rtt_length(Config) -> end, %% before stream shutdown, {error, invalid_state} = quicer:getopt(Stm, param_stream_0rtt_length), - quicer:shutdown_stream(Stm), - {ok, Val} = quicer:getopt(Stm, param_stream_0rtt_length), - ?assert(is_integer(Val)), + quicer:async_shutdown_stream(Stm), + case quicer:getopt(Stm, param_stream_0rtt_length) of + {ok, Val} -> ?assert(is_integer(Val)); + {error, invalid_state} -> ok; + {error, invalid_parameter} -> ok + end, + quicer:close_connection(Conn), SPid ! done, ensure_server_exit_normal(Ref) after 5000 -> @@ -1038,8 +1052,6 @@ tc_get_stream_ideal_sndbuff_size(Config) -> {ok, Val} = quicer:getopt(Stm, param_stream_ideal_send_buffer_size), ?assert(is_integer(Val)), ok = quicer:shutdown_stream(Stm), - {ok, Val2} = quicer:getopt(Stm, param_stream_ideal_send_buffer_size), - ?assert(is_integer(Val2)), SPid ! done, ensure_server_exit_normal(Ref) after 5000 -> @@ -1113,7 +1125,7 @@ tc_peername_v6(Config) -> tc_peername_v4(Config) -> Port = select_port(), Owner = self(), - {_SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), + {SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), receive listener_ready -> {ok, Conn} = quicer:connect("127.0.0.1", Port, default_conn_opts(), 5000), @@ -1125,7 +1137,8 @@ tc_peername_v4(Config) -> true = is_integer(RPort), ct:pal("addr is ~p", [Addr]), "127.0.0.1" = inet:ntoa(Addr), - ok = quicer:close_connection(Conn) + ok = quicer:close_connection(Conn), + SPid ! done %{error, _} = quicer:peername(Conn) after 5000 -> ct:fail("listener_timeout") @@ -1161,11 +1174,10 @@ tc_alpn_mismatch(Config) -> receive ok -> ct:fail("illegal connection"); - {error, transport_down} -> - ok - after 1000 -> - SPid ! done - end + {error, transport_down, #{error := 376, status := alpn_neg_failure}} -> + ok + end, + SPid ! done after 1000 -> ct:fail("timeout") end. @@ -1526,6 +1538,7 @@ tc_setopt_stream_unsupp_opts(Config) -> {ok, <<"ping">>} = quicer:recv(Stm, 0), % try to set priority out of range {error, param_error} = quicer:setopt(Stm, param_stream_priority, 65536), + quicer:shutdown_stream(Stm), SPid ! done, ensure_server_exit_normal(Ref) after 5000 -> @@ -1724,11 +1737,13 @@ tc_stream_open_flag_unidirectional(Config) -> -> ct:pal("stream is closed due to connecion idle") end, ?assert(is_integer(Rid)), - ?assert(Rid =/= 0). + ?assert(Rid =/= 0), + quicer:close_connection(Conn). tc_stream_start_flag_fail_blocked(Config) -> Port = select_port(), application:ensure_all_started(quicer), + %% Given a server with 0 allowed remote bidi stream. ListenerOpts = [{conn_acceptors, 32}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE} | lists:keyreplace(peer_bidi_stream_count, 1, default_listen_opts(Config), {peer_bidi_stream_count,0})], ConnectionOpts = [ {conn_callback, quicer_server_conn_callback} @@ -1740,16 +1755,27 @@ tc_stream_start_flag_fail_blocked(Config) -> ct:pal("Listener Options: ~p", [Options]), {ok, _QuicApp} = quicer:spawn_listener(mqtt, Port, Options), {ok, Conn} = quicer:connect("localhost", Port, default_conn_opts(), 5000), + %% When a client tries to start a bidi stream with flag "QUIC_STREAM_START_FLAG_FAIL_BLOCKED" {ok, Stm} = quicer:start_stream(Conn, [ {active, 3}, {start_flag, ?QUIC_STREAM_START_FLAG_FAIL_BLOCKED} , {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE} ]), {ok, Rid} = quicer:get_stream_rid(Stm), - {ok, 5} = quicer:async_send(Stm, <<"ping1">>), + case quicer:async_send(Stm, <<"ping1">>) of + {ok, 5} -> + ok; + {error, closed} -> + ok; + {error, stm_send_error, invalid_state} -> + %% Deps on the timing + ok + end, receive {quic, <<"ping1">>, Stm, _} -> ct:fail("Should not get ping1 due to rate limiter"); {quic, start_completed, Stm, #{status := stream_limit_reached, stream_id := StreamID}} -> + %% Then stream start should fail with reason stream_limit_reached + quicer:close_stream(Stm, ?QUIC_STREAM_SHUTDOWN_FLAG_ABORT bor ?QUIC_STREAM_SHUTDOWN_FLAG_IMMEDIATE, 0, 1000), ct:pal("Stream ~p limit reached", [StreamID]); {quic, start_completed, Stm, #{status := AtomStatus, stream_id := StreamID, is_peer_accepted := _PeerAccepted} @@ -1757,6 +1783,7 @@ tc_stream_start_flag_fail_blocked(Config) -> ct:fail("Stream ~pstart complete with unexpect reason: ~p", [StreamID, AtomStatus]) end, + %% Then stream is closed automatically receive {quic, stream_closed, Stm, _} -> ct:failed("Stream ~p is closed but shouldn't since QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL is unset", [Stm]); @@ -1767,8 +1794,8 @@ tc_stream_start_flag_fail_blocked(Config) -> {quic, closed, Conn, _Flags} -> ct:pal("Connecion is closed ~p", [Conn]) end, - ?assert(is_integer(Rid)), - ?assert(Rid =/= 0). + quicer:terminate_listener(mqtt), + ?assert(is_integer(Rid)). tc_stream_start_flag_immediate(Config) -> Port = select_port(), @@ -1803,6 +1830,7 @@ tc_stream_start_flag_immediate(Config) -> tc_stream_start_flag_shutdown_on_fail(Config) -> Port = select_port(), application:ensure_all_started(quicer), + %% Given a server with 0 allowed remote bidi stream. ListenerOpts = [{conn_acceptors, 32}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE} | lists:keyreplace(peer_bidi_stream_count, 1, default_listen_opts(Config), {peer_bidi_stream_count,0})], ConnectionOpts = [ {conn_callback, quicer_server_conn_callback} @@ -1814,6 +1842,7 @@ tc_stream_start_flag_shutdown_on_fail(Config) -> ct:pal("Listener Options: ~p", [Options]), {ok, _QuicApp} = quicer:spawn_listener(mqtt, Port, Options), {ok, Conn} = quicer:connect("localhost", Port, default_conn_opts(), 5000), + %% When a client tries to start a bidi stream with flag "QUIC_STREAM_START_FLAG_FAIL_BLOCKED" unset {ok, Stm} = quicer:start_stream(Conn, [ {active, 3}, {start_flag, ?QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL bor ?QUIC_STREAM_START_FLAG_FAIL_BLOCKED } @@ -1822,6 +1851,7 @@ tc_stream_start_flag_shutdown_on_fail(Config) -> {ok, Rid} = quicer:get_stream_rid(Stm), case quicer:async_send(Stm, <<"ping1">>) of {ok, 5} -> ok; + {error, closed} -> ok; {error, stm_send_error, invalid_state} -> ok %% already closed end, receive @@ -1845,8 +1875,7 @@ tc_stream_start_flag_shutdown_on_fail(Config) -> Other -> ct:fail("Unexpected event ~p after stream start complete", [Other]) end, - ?assert(is_integer(Rid)), - ?assert(Rid =/= 0). + ?assert(is_integer(Rid)). tc_stream_start_flag_indicate_peer_accept_1(Config) -> Port = select_port(), @@ -2351,10 +2380,14 @@ tc_direct_send_over_conn_fail(Config) -> quicer:shutdown_connection(Conn), %% csend over a closed conn - {error, stm_open_error, invalid_state} = - quicer:async_csend(Conn, <<"ping2">>, [{active, true}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE}, - {open_flag, ?QUIC_STREAM_OPEN_FLAG_UNIDIRECTIONAL} - ], ?QUIC_SEND_FLAG_ALLOW_0_RTT), + + case quicer:async_csend(Conn, <<"ping2">>, [{active, true}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE}, + {open_flag, ?QUIC_STREAM_OPEN_FLAG_UNIDIRECTIONAL} + ], ?QUIC_SEND_FLAG_ALLOW_0_RTT) of + {error, closed} -> ok; + {error, stm_open_error, invalid_parameter} -> ok; + {error, stm_open_error, invalid_state} -> ok + end, receive {quic, start_completed, Stm0, #{status := StartStatus, stream_id := StreamId2}} -> @@ -2369,6 +2402,7 @@ tc_direct_send_over_conn_fail(Config) -> #{status := StartStatusX, stream_id := StreamIdX}} when StmX =/= Stm0 -> ct:fail("Stream id: ~p started: ~p", [StreamIdX, StartStatusX]) after 100 -> + quicer:close_connection(Conn), ok end. @@ -2553,6 +2587,8 @@ echo_server_stm_loop(L, Conn, Stms) -> {error, cancelled} -> ct:pal("echo server: send cancelled: ~p ", [Bin]), cancelled; + {error, closed} -> + closed; {ok, _} -> ok end, @@ -2743,11 +2779,14 @@ simple_conn_server_client_cert_loop(L, Conn, Owner) -> conn_server_with(Owner, Port, Opts) -> {ok, L} = quicer:listen(Port, Opts), Owner ! listener_ready, - {ok, Conn} = quicer:accept(L, [], 10000), - {ok, Conn} = quicer:handshake(Conn), + case quicer:accept(L, [], 10000) of + {error, _} -> + quicer:close_listener(L); + {ok, Conn} -> + {ok, Conn} = quicer:handshake(Conn) + end, receive done -> - quicer:close_listener(L), - ok + quicer:close_listener(L) end. simple_stream_server(Owner, Config, Port) -> @@ -2758,7 +2797,12 @@ simple_stream_server(Owner, Config, Port) -> {ok, Conn} = quicer:handshake(Conn), receive {quic, new_stream, Stream, _Props} -> - {ok, StreamId} = quicer:get_stream_id(Stream), + StreamId = case quicer:get_stream_id(Stream) of + {ok, Stm} -> + Stm; + {error, _} -> + simple_stream_server_exit(L) + end, ct:pal("New StreamID: ~p", [StreamId]), receive {quic, shutdown, Conn, _ErrorCode} -> @@ -2767,7 +2811,7 @@ simple_stream_server(Owner, Config, Port) -> {quic, peer_send_shutdown, Stream, undefined} -> quicer:close_stream(Stream); done -> - exit(normal) + simple_stream_server_exit(L) end; {quic, shutdown, Conn, _ErrorCode} -> ct:pal("Received Conn close for ~p", [Conn]), @@ -2780,6 +2824,9 @@ simple_stream_server(Owner, Config, Port) -> done -> ok end, + simple_stream_server_exit(L). + +simple_stream_server_exit(L) -> quicer:close_listener(L). diff --git a/test/quicer_connection_SUITE.erl b/test/quicer_connection_SUITE.erl index 713a2957..fbe31a95 100644 --- a/test/quicer_connection_SUITE.erl +++ b/test/quicer_connection_SUITE.erl @@ -58,6 +58,8 @@ init_per_suite(Config) -> %% @end %%-------------------------------------------------------------------- end_per_suite(_Config) -> + code:purge(quicer_nif), + code:delete(quicer_nif), ok. %%-------------------------------------------------------------------- @@ -83,7 +85,9 @@ init_per_group(suite_reg, Config) -> %% @end %%-------------------------------------------------------------------- end_per_group(suite_reg, Config) -> - quicer:shutdown_registration(proplists:get_value(quic_registration, Config)); + Reg = proplists:get_value(quic_registration, Config), + quicer:shutdown_registration(Reg), + ok = quicer:close_registration(Reg); end_per_group(_GroupName, _Config) -> ok. @@ -96,6 +100,8 @@ end_per_group(_GroupName, _Config) -> %% @end %%-------------------------------------------------------------------- init_per_testcase(_TestCase, Config) -> + application:ensure_all_started(quicer), + quicer_test_lib:cleanup_msquic(), Config. %%-------------------------------------------------------------------- @@ -107,6 +113,8 @@ init_per_testcase(_TestCase, Config) -> %% @end %%-------------------------------------------------------------------- end_per_testcase(_TestCase, _Config) -> + quicer_test_lib:report_active_connections(), + ct:pal("Counters ~p", [quicer:perf_counters()]), ok. %%-------------------------------------------------------------------- @@ -572,7 +580,7 @@ tc_conn_controlling_process(Config) -> tc_conn_opt_ideal_processor(Config) -> Port = select_port(), Owner = self(), - {_SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), + {SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), receive listener_ready -> {ok, Conn} = quicer:connect("127.0.0.1", Port, default_conn_opts(Config), 5000), @@ -580,7 +588,8 @@ tc_conn_opt_ideal_processor(Config) -> {ok, 4} = quicer:send(Stm, <<"ping">>), {ok, Processor} = quicer:getopt(Conn, param_conn_ideal_processor), ?assert(is_integer(Processor)), - ok = quicer:close_connection(Conn) + ok = quicer:close_connection(Conn), + SPid ! done after 5000 -> ct:fail("listener_timeout") end. @@ -588,7 +597,7 @@ tc_conn_opt_ideal_processor(Config) -> tc_conn_opt_share_udp_binding(Config) -> Port = select_port(), Owner = self(), - {_SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), + {SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), receive listener_ready -> {ok, Conn} = quicer:connect("127.0.0.1", Port, default_conn_opts(Config), 5000), @@ -598,7 +607,8 @@ tc_conn_opt_share_udp_binding(Config) -> ?assert(is_boolean(IsShared)), {error, invalid_state} = quicer:setopt(Conn, param_conn_share_udp_binding, not IsShared), {ok, IsShared} = quicer:getopt(Conn, param_conn_share_udp_binding), - ok = quicer:close_connection(Conn) + ok = quicer:close_connection(Conn), + SPid ! done after 5000 -> ct:fail("listener_timeout") end. @@ -606,7 +616,7 @@ tc_conn_opt_share_udp_binding(Config) -> tc_conn_opt_local_bidi_stream_count(Config) -> Port = select_port(), Owner = self(), - {_SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), + {SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), receive listener_ready -> {ok, Conn} = quicer:connect("127.0.0.1", Port, default_conn_opts(Config), 5000), @@ -615,7 +625,8 @@ tc_conn_opt_local_bidi_stream_count(Config) -> {ok, Cnt} = quicer:getopt(Conn, param_conn_local_bidi_stream_count), ?assert(is_integer(Cnt)), {error, invalid_parameter} = quicer:setopt(Conn, param_conn_local_bidi_stream_count, Cnt + 2), - ok = quicer:close_connection(Conn) + ok = quicer:close_connection(Conn), + SPid ! done after 5000 -> ct:fail("listener_timeout") end. @@ -623,7 +634,7 @@ tc_conn_opt_local_bidi_stream_count(Config) -> tc_conn_opt_local_uni_stream_count(Config) -> Port = select_port(), Owner = self(), - {_SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), + {SPid, _Ref} = spawn_monitor(fun() -> echo_server(Owner, Config, Port) end), receive listener_ready -> {ok, Conn} = quicer:connect("127.0.0.1", Port, default_conn_opts(Config), 5000), @@ -632,7 +643,8 @@ tc_conn_opt_local_uni_stream_count(Config) -> {ok, Cnt} = quicer:getopt(Conn, param_conn_local_unidi_stream_count), ?assert(is_integer(Cnt)), {error, invalid_parameter} = quicer:setopt(Conn, param_conn_local_unidi_stream_count, Cnt + 2), - ok = quicer:close_connection(Conn) + ok = quicer:close_connection(Conn), + SPid ! done after 5000 -> ct:fail("listener_timeout") end. @@ -788,7 +800,8 @@ echo_server(Owner, Config, Port)-> end end, ct:pal("echo server stream accepted", []), - echo_server_stm_loop(L, Conn, [Stm]); + catch echo_server_stm_loop(L, Conn, [Stm]), + quicer:close_listener(L); {error, listener_start_error, 200000002} -> ct:pal("echo_server: listener_start_error", []), timer:sleep(100), @@ -815,6 +828,8 @@ echo_server_stm_loop(L, Conn, Stms) -> {error, cancelled} -> ct:pal("echo server: send cancelled: ~p ", [Bin]), cancelled; + {error, closed} -> + closed; {ok, _} -> ok end, @@ -868,8 +883,7 @@ echo_server_stm_loop(L, Conn, Stms) -> echo_server_stm_loop(L, Conn, NewStmList); done -> ct:pal("echo server shutting down", []), - quicer:async_close_connection(Conn), - quicer:close_listener(L) + quicer:async_close_connection(Conn) end. default_conn_opts(Config) -> diff --git a/test/quicer_echo_server_stream_callback.erl b/test/quicer_echo_server_stream_callback.erl index 9084f135..677ae3c0 100644 --- a/test/quicer_echo_server_stream_callback.erl +++ b/test/quicer_echo_server_stream_callback.erl @@ -94,13 +94,23 @@ handle_stream_data(Stream, Bin, _Opts, #{ sent_bytes := Cnt } = State) -> case maps:get(is_echo_new_stream, StreamOpts, false) of false -> - {ok, Size} = quicer:send(Stream, echo_msg(Bin, State)), - {ok, State#{ sent_bytes => Cnt + Size }}; + case quicer:send(Stream, echo_msg(Bin, State)) of + {ok, Size} -> + {ok, State#{ sent_bytes => Cnt + Size }}; + _ -> + %% handle error in test aborted. + {ok, State} + end; true -> %% echo reply with a new stream from server to client. {ok, EchoStream} = quicer:start_stream(Conn, StreamOpts), - {ok, Size} = quicer:send(EchoStream, echo_msg(Bin, State)), - {ok, State#{ sent_bytes => Cnt + Size, echo_stream => EchoStream }} + case quicer:send(EchoStream, echo_msg(Bin, State)) of + {ok, Size} -> + {ok, State#{ sent_bytes => Cnt + Size, echo_stream => EchoStream }}; + _ -> + %% handle error in test aborted. + {ok, State} + end end; handle_stream_data(Stream, Bin, _Opts, #{ sent_bytes := Cnt , echo_stream := _Ignore diff --git a/test/quicer_listener_SUITE.erl b/test/quicer_listener_SUITE.erl index 36ef1bc3..b295baf5 100644 --- a/test/quicer_listener_SUITE.erl +++ b/test/quicer_listener_SUITE.erl @@ -45,6 +45,7 @@ suite() -> %% @end %%-------------------------------------------------------------------- init_per_suite(Config) -> + application:ensure_all_started(quicer), quicer_test_lib:generate_tls_certs(Config), Config. @@ -79,7 +80,9 @@ init_per_group(suite_reg, Config) -> %% @end %%-------------------------------------------------------------------- end_per_group(suite_reg, Config) -> - quicer:shutdown_registration(proplists:get_value(quic_registration, Config)); + Reg = proplists:get_value(quic_registration, Config), + quicer:shutdown_registration(Reg), + ok = quicer:close_registration(Reg); end_per_group(_GroupName, _Config) -> ok. @@ -93,6 +96,7 @@ end_per_group(_GroupName, _Config) -> %%-------------------------------------------------------------------- init_per_testcase(_TestCase, Config) -> application:ensure_all_started(quicer), + quicer_test_lib:cleanup_msquic(), Config. %%-------------------------------------------------------------------- @@ -106,6 +110,7 @@ init_per_testcase(_TestCase, Config) -> end_per_testcase(_TestCase, Config) -> RegH = proplists:get_value(quic_registration, Config, global), [quicer:close_listener(L, 1000) || L <- quicer:get_listeners(RegH)], + quicer_test_lib:report_active_connections(), ok. %%-------------------------------------------------------------------- diff --git a/test/quicer_reg_SUITE.erl b/test/quicer_reg_SUITE.erl index 91d729bf..32f7b56a 100644 --- a/test/quicer_reg_SUITE.erl +++ b/test/quicer_reg_SUITE.erl @@ -90,7 +90,7 @@ init_per_testcase(_TestCase, Config) -> %%-------------------------------------------------------------------- end_per_testcase(_TestCase, _Config) -> erlang:garbage_collect(self(), [{type, major}]), - timer:sleep(1000), + quicer_test_lib:report_active_connections(), ok. %%-------------------------------------------------------------------- @@ -167,7 +167,8 @@ tc_shutdown_3_abnormal(_Config) -> {ok, Reg} = quicer:new_registration(Name, Profile), ?assertEqual({error, badarg}, quicer:shutdown_registration(Reg, 1, 2)), ?assertEqual({error, badarg}, quicer:shutdown_registration(Reg, 1, foo)), - ?assertEqual({error, badarg}, quicer:shutdown_registration(Reg, true, -1)). + ?assertEqual({error, badarg}, quicer:shutdown_registration(Reg, true, -1)), + ok = quicer:shutdown_registration(Reg). tc_shutdown_ok(_Config) -> Name = atom_to_list(?FUNCTION_NAME), diff --git a/test/quicer_snb_SUITE.erl b/test/quicer_snb_SUITE.erl index f4ab9ba3..a08b0e14 100644 --- a/test/quicer_snb_SUITE.erl +++ b/test/quicer_snb_SUITE.erl @@ -97,6 +97,7 @@ init_per_suite(Config) -> %% @end %%-------------------------------------------------------------------- end_per_suite(_Config) -> + quicer_test_lib:report_active_connections(), ok. %%-------------------------------------------------------------------- @@ -135,6 +136,7 @@ init_per_testcase(tc_listener_inval_local_addr, Config) -> Config end; init_per_testcase(_TestCase, Config) -> + quicer_test_lib:cleanup_msquic(), Config. %%-------------------------------------------------------------------- @@ -148,8 +150,8 @@ init_per_testcase(_TestCase, Config) -> end_per_testcase(_TestCase, _Config) -> quicer:terminate_listener(mqtt), snabbkaffe:cleanup(), - Unhandled = quicer_test_lib:receive_all(), - Unhandled =/= [] andalso ct:comment("What left in the message queue: ~p", [Unhandled]), + quicer_test_lib:report_unhandled_messages(), + quicer_test_lib:report_active_connections(), ok. %%-------------------------------------------------------------------- @@ -414,6 +416,7 @@ tc_conn_owner_down(Config) -> begin {ok, _QuicApp} = quicer_start_listener(mqtt, Port, Options), {ok, Conn} = quicer:connect("localhost", Port, default_conn_opts(), 5000), + {ok, CRid} = quicer:get_conn_rid(Conn), {ok, Stm} = quicer:start_stream(Conn, [{active, false}]), {ok, 4} = quicer:send(Stm, <<"ping">>), {ok, <<"ping">>} = quicer:recv(Stm, 4), @@ -422,6 +425,12 @@ tc_conn_owner_down(Config) -> receive down -> ok end end), quicer:controlling_process(Conn, Pid), + + ?assert(timeout =/= ?block_until(#{ ?snk_kind := debug + , function := "connection_controlling_process" + , tag := "exit" + , resource_id := CRid + }, 1000, 1000)), Pid ! down, ?assert(timeout =/= ?block_until( @@ -438,7 +447,7 @@ tc_conn_owner_down(Config) -> #{?snk_kind := debug, event := closed, module := quicer_connection}, 1000, 1000)), ct:pal("stop listener"), ok = quicer:terminate_listener(mqtt), - {ok, CRid} = quicer:get_conn_rid(Conn), + {CRid, SRid} end, fun(Result, Trace) -> @@ -826,7 +835,8 @@ tc_conn_idle_close(Config) -> end, case quicer:send(Stm, <<"ping2">>) of {error, stm_send_error, invalid_state} -> ok; - {error, cancelled} -> ok + {error, cancelled} -> ok; + {error, closed} -> ok end, ?block_until( @@ -883,9 +893,9 @@ tc_conn_gc(Config) -> | default_stream_opts() ], Options = {ListenerOpts, ConnectionOpts, StreamOpts}, ct:pal("Listener Options: ~p", [Options]), - ?check_trace(#{timetrap => 1000}, + ?check_trace(#{timetrap => 100000}, begin - %% Spawn a process that will die without handler cleanups + %% Spawn a process that will die without handle cleanups %% The dead process should trigger a connection close %% The dead process should trigger a GC {ok, _QuicApp} = quicer_start_listener(mqtt, Port, Options), @@ -931,9 +941,10 @@ tc_conn_gc(Config) -> {ok, _} = ?block_until(#{ ?snk_kind := debug , context := "callback" , function := "resource_conn_dealloc_callback" - , resource_id := CRid + , resource_id := 0 , tag := "end"}, 5000, 1000), + timer:sleep(1000), {SRid, CRid} end, fun({_SRid, CRid}, Trace) -> @@ -951,7 +962,7 @@ tc_conn_gc(Config) -> #{ ?snk_kind := debug , context := "callback" , function := "resource_conn_dealloc_callback" - , resource_id := CRid + , resource_id := 0 , tag := "end"}, Trace)), ?assert(?strict_causality(#{ ?snk_kind := debug @@ -984,7 +995,7 @@ tc_conn_gc(Config) -> ), ?assertEqual(1, length([ E || #{ function := "resource_conn_dealloc_callback" , resource_id := Rid - , tag := "end"} = E <- TraceEvents, Rid == CRid]) + , tag := "end"} = E <- TraceEvents, Rid == 0]) ) end), ct:pal("stop listener"), @@ -1610,10 +1621,7 @@ tc_listener_no_acceptor(Config) -> begin {ok, _QuicApp} = quicer_start_listener(mqtt, Port, Options), {error, transport_down, #{status := connection_refused}} - = quicer:connect("localhost", Port, default_conn_opts(), 5000), - ct:pal("stop listener"), - ok = quicer:terminate_listener(mqtt), - timer:sleep(5000) + = quicer:connect("localhost", Port, default_conn_opts(), 5000) end, fun(_Result, Trace) -> ct:pal("Trace is ~p", [Trace]), @@ -1631,6 +1639,8 @@ tc_listener_no_acceptor(Config) -> }, Trace)) end), + ct:pal("stop listener"), + ok = quicer:terminate_listener(mqtt), ok. %% @doc this triggers listener start fail @@ -1714,6 +1724,7 @@ tc_conn_stop_notify_acceptor(Config) -> {error, closed} -> ok; {ok, _Stream} -> ok end, + quicer:close_listener(Listener), exit({normal, Acceptors}) end), receive {SPid, ready} -> ok end, diff --git a/test/quicer_test_lib.erl b/test/quicer_test_lib.erl index a8c8cfdb..28d29695 100644 --- a/test/quicer_test_lib.erl +++ b/test/quicer_test_lib.erl @@ -30,7 +30,12 @@ select_free_port/1, flush/1, ensure_server_exit_normal/1, - ensure_server_exit_normal/2 + ensure_server_exit_normal/2, + + report_active_connections/0, + report_active_connections/1, + + report_unhandled_messages/0 ]). @@ -40,6 +45,13 @@ , default_stream_opts/0 ]). +%% cleanups +-export([ reset_global_reg/0 + , shutdown_all_listeners/0 + , cleanup_msquic/0 + ]). + + %% ct helper -export([all_tcs/1]). @@ -345,6 +357,36 @@ ensure_server_exit_normal(MonRef, Timeout) -> ct:fail("server still running", []) end. +-spec report_active_connections() -> _. +report_active_connections() -> + report_active_connections(fun ct:comment/2). +report_active_connections(LogFun) -> + erlang:garbage_collect(), + {ok, Cnts} = quicer:perf_counters(), + ActiveStrms = proplists:get_value(strm_active, Cnts), + ActiveConns = proplists:get_value(conn_active, Cnts), + 0 =/= (ActiveStrms + ActiveConns) andalso + LogFun("active conns: ~p, strms: ~p", [ActiveConns, ActiveStrms]). + +-spec report_unhandled_messages() -> ok. +report_unhandled_messages() -> + Unhandled = quicer_test_lib:receive_all(), + Unhandled =/= [] andalso + ct:comment("What left in the message queue: ~p", [Unhandled]). + +-spec cleanup_msquic() -> ok. +cleanup_msquic() -> + shutdown_all_listeners(), + reset_global_reg(), + ok. + +reset_global_reg()-> + quicer:reg_close(), + quicer:reg_open(). + +shutdown_all_listeners() -> + lists:foreach(fun quicer:shutdown_listener/1, + quicer:listeners()). %%%_* Emacs ==================================================================== %%% Local Variables: diff --git a/test/quicer_upgrade_SUITE.erl b/test/quicer_upgrade_SUITE.erl index 78028f36..a15cfde9 100644 --- a/test/quicer_upgrade_SUITE.erl +++ b/test/quicer_upgrade_SUITE.erl @@ -93,6 +93,7 @@ init_per_testcase(TestCase, Config) -> %% @end %%-------------------------------------------------------------------- end_per_testcase(_TestCase, _Config) -> + quicer_test_lib:report_active_connections(), ok. %%-------------------------------------------------------------------- From 1600b7440c3f32653ea1f5fe2c445f88058fe710 Mon Sep 17 00:00:00 2001 From: William Yang Date: Fri, 6 Oct 2023 16:01:28 +0200 Subject: [PATCH 08/22] refactor: closing msquic handles early, part 1 --- c_src/quicer_connection.c | 30 +++++++++++++++++++++++++++++- c_src/quicer_ctx.c | 4 +--- c_src/quicer_listener.c | 16 +++++++++++++--- c_src/quicer_nif.c | 30 ++++++++++++++++++------------ c_src/quicer_stream.c | 15 +++++++++++---- c_src/quicer_vsn.h.in | 2 ++ 6 files changed, 74 insertions(+), 23 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..707af5ce 100644 --- a/c_src/quicer_ctx.c +++ b/c_src/quicer_ctx.c @@ -254,10 +254,8 @@ deinit_s_ctx(QuicerStreamCTX *s_ctx) void destroy_s_ctx(QuicerStreamCTX *s_ctx) { + assert(!s_ctx->Stream); 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_release_resource(s_ctx); } diff --git a/c_src/quicer_listener.c b/c_src/quicer_listener.c index 2e1d1d2f..faa02fce 100644 --- a/c_src/quicer_listener.c +++ b/c_src/quicer_listener.c @@ -23,6 +23,7 @@ limitations under the License. #include extern QuicerRegistrationCTX *G_r_ctx; +extern pthread_mutex_t GRegLock; BOOLEAN parse_registration(ErlNifEnv *env, ERL_NIF_TERM options, @@ -576,8 +577,15 @@ ERL_NIF_TERM get_listenersX(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { QuicerRegistrationCTX *r_ctx = NULL; - if (argc == 0) + ERL_NIF_TERM res = enif_make_list(env, 0); + if (argc == 0) // use global registration { + pthread_mutex_lock(&GRegLock); + if (!G_r_ctx) + { + pthread_mutex_unlock(&GRegLock); + return res; + } r_ctx = G_r_ctx; } else @@ -587,8 +595,6 @@ get_listenersX(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) return ERROR_TUPLE_2(ATOM_BADARG); } } - ERL_NIF_TERM res = enif_make_list(env, 0); - enif_mutex_lock(r_ctx->lock); CXPLAT_LIST_ENTRY *Entry = r_ctx->Listeners.Flink; while (Entry != &r_ctx->Listeners) @@ -600,5 +606,9 @@ get_listenersX(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) } enif_mutex_unlock(r_ctx->lock); + if (argc == 0) // use global registration + { + pthread_mutex_unlock(&GRegLock); + } return res; } 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..50b7d758 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); } @@ -228,6 +230,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 +238,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 +346,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 +358,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 +667,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 +1008,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_ From 8d5176a632d3f7ff18eb24ff45aba7d7ccb9730f Mon Sep 17 00:00:00 2001 From: William Yang Date: Fri, 6 Oct 2023 22:17:08 +0200 Subject: [PATCH 09/22] fix: check lib isn't closed while getopt global --- c_src/quicer_config.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/c_src/quicer_config.c b/c_src/quicer_config.c index f970873f..60174c94 100644 --- a/c_src/quicer_config.c +++ b/c_src/quicer_config.c @@ -21,6 +21,7 @@ limitations under the License. #include extern QuicerRegistrationCTX *G_r_ctx; +extern pthread_mutex_t MsQuicLock; static ERL_NIF_TERM get_stream_opt(ErlNifEnv *env, QuicerStreamCTX *s_ctx, @@ -768,7 +769,14 @@ getopt3(ErlNifEnv *env, if (IS_SAME_TERM(ATOM_QUIC_GLOBAL, ctx)) { - res = get_global_opt(env, NULL, eopt); + pthread_mutex_lock(&MsQuicLock); + // Get global option without using any ctx + // We are risking using a closed lib + if (MsQuic) + { + res = get_global_opt(env, NULL, eopt); + } + pthread_mutex_unlock(&MsQuicLock); } else if (enif_get_resource(env, ctx, ctx_stream_t, &q_ctx)) { From c2bd7179008111b0cd70c5e6f48beb9549239159 Mon Sep 17 00:00:00 2001 From: William Yang Date: Fri, 6 Oct 2023 22:17:41 +0200 Subject: [PATCH 10/22] upgrade: tear down msquic when unload quicer_nif --- c_src/quicer_config.c | 6 +++--- c_src/quicer_nif.c | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/c_src/quicer_config.c b/c_src/quicer_config.c index 60174c94..d04739a2 100644 --- a/c_src/quicer_config.c +++ b/c_src/quicer_config.c @@ -773,9 +773,9 @@ getopt3(ErlNifEnv *env, // Get global option without using any ctx // We are risking using a closed lib if (MsQuic) - { - res = get_global_opt(env, NULL, eopt); - } + { + res = get_global_opt(env, NULL, eopt); + } pthread_mutex_unlock(&MsQuicLock); } else if (enif_get_resource(env, ctx, ctx_stream_t, &q_ctx)) diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index 8b2dca67..21f09b10 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -1147,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 From 1fefbaa63c6e6935474042b47dec4866a2575924 Mon Sep 17 00:00:00 2001 From: William Yang Date: Sat, 7 Oct 2023 18:30:52 +0200 Subject: [PATCH 11/22] fix(listener): credconfig leak --- c_src/quicer_listener.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/c_src/quicer_listener.c b/c_src/quicer_listener.c index faa02fce..58d46496 100644 --- a/c_src/quicer_listener.c +++ b/c_src/quicer_listener.c @@ -280,6 +280,7 @@ listen2(ErlNifEnv *env, __unused_parm__ int argc, const ERL_NIF_TERM argv[]) { // TLS opt error not file content error free(cacertfile); + free_certificate(&CredConfig); return ERROR_TUPLE_2(ATOM_CACERTFILE); } @@ -289,6 +290,7 @@ listen2(ErlNifEnv *env, __unused_parm__ int argc, const ERL_NIF_TERM argv[]) if (!l_ctx) { free(cacertfile); + free_certificate(&CredConfig); return ERROR_TUPLE_2(ATOM_ERROR_NOT_ENOUGH_MEMORY); } @@ -307,6 +309,10 @@ listen2(ErlNifEnv *env, __unused_parm__ int argc, const ERL_NIF_TERM argv[]) goto exit; } } + else + { // since we don't use cacertfile, free it + free(cacertfile); + } // Set owner for l_ctx if (!enif_self(env, &(l_ctx->listenerPid))) From f5e838c5751578e24925e1139529b4d8749384d5 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 9 Oct 2023 09:21:46 +0200 Subject: [PATCH 12/22] ci: apply job timeout --- .github/workflows/main.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index e2c86c32..028c90df 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -3,6 +3,7 @@ on: [push, pull_request] jobs: mac: + timeout-minutes: 45 strategy: fail-fast: false matrix: @@ -62,6 +63,7 @@ jobs: linux: runs-on: ubuntu-22.04 + timeout-minutes: 25 strategy: fail-fast: false matrix: From e659a269a8b00db810671bb1ab4377fc53ac3dce Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 9 Oct 2023 14:50:44 +0200 Subject: [PATCH 13/22] feat(api): new API close_registration --- c_src/quicer_nif.c | 1 + c_src/quicer_reg.c | 20 ++++++++++++++++++++ c_src/quicer_reg.h | 3 +++ src/quicer.erl | 7 +++++++ src/quicer_nif.erl | 5 +++++ 5 files changed, 36 insertions(+) diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index 21f09b10..e99ab381 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -1528,6 +1528,7 @@ static ErlNifFunc nif_funcs[] = { { "new_registration", 2, new_registration2, 0}, { "shutdown_registration", 1, shutdown_registration_x, 0}, { "shutdown_registration", 3, shutdown_registration_x, 0}, + { "close_registration", 1, close_registration, 0}, { "get_registration_name", 1, get_registration_name1, 0}, { "listen", 2, listen2, 0}, { "start_listener", 3, start_listener3, 0}, diff --git a/c_src/quicer_reg.c b/c_src/quicer_reg.c index 7e51b559..7377edda 100644 --- a/c_src/quicer_reg.c +++ b/c_src/quicer_reg.c @@ -199,6 +199,26 @@ shutdown_registration_x(ErlNifEnv *env, int argc, const ERL_NIF_TERM *argv) return ATOM_OK; } +ERL_NIF_TERM +close_registration(ErlNifEnv *env, + __unused_parm__ int argc, + const ERL_NIF_TERM argv[]) +{ + QuicerRegistrationCTX *r_ctx = NULL; + HQUIC Registration = NULL; + ERL_NIF_TERM ectx = argv[0]; + if (!enif_get_resource(env, ectx, ctx_reg_t, (void **)&r_ctx)) + { + return ERROR_TUPLE_2(ATOM_BADARG); + } + enif_mutex_lock(r_ctx->lock); + Registration = r_ctx->Registration; + r_ctx->Registration = NULL; + enif_mutex_unlock(r_ctx->lock); + MsQuic->RegistrationClose(Registration); + return ATOM_OK; +} + ERL_NIF_TERM get_registration_name1(ErlNifEnv *env, __unused_parm__ int argc, diff --git a/c_src/quicer_reg.h b/c_src/quicer_reg.h index c326530d..9fe6eaa2 100644 --- a/c_src/quicer_reg.h +++ b/c_src/quicer_reg.h @@ -29,6 +29,9 @@ new_registration2(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); ERL_NIF_TERM shutdown_registration_x(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); +ERL_NIF_TERM +close_registration(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); + ERL_NIF_TERM get_registration_name1(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]); diff --git a/src/quicer.erl b/src/quicer.erl index 655b1fdc..cd531019 100644 --- a/src/quicer.erl +++ b/src/quicer.erl @@ -114,6 +114,7 @@ , open_connection/0 , get_listeners/0 , get_listeners/1 + , close_registration/1 ]). -export([ spawn_listener/3 %% start application over quic @@ -172,6 +173,12 @@ shutdown_registration(Handle) -> shutdown_registration(Handle, IsSilent, ErrCode) -> quicer_nif:shutdown_registration(Handle, IsSilent, ErrCode). +%% @doc close a registration. +-spec close_registration(Handle) -> + quicer_nif:close_registration(Handle). +close_registration(Handle) -> + quicer_nif:close_registration(Handle). + %% @doc get registration name -spec get_registration_name(Handle) -> quicer_nif:get_registration_name(Handle). diff --git a/src/quicer_nif.erl b/src/quicer_nif.erl index 451b0caf..a10aa547 100644 --- a/src/quicer_nif.erl +++ b/src/quicer_nif.erl @@ -22,6 +22,7 @@ , new_registration/2 , shutdown_registration/1 , shutdown_registration/3 + , close_registration/1 , get_registration_name/1 , listen/2 , start_listener/3 @@ -156,6 +157,10 @@ shutdown_registration(_Handle) -> shutdown_registration(_Handle, _IsSilent, _ErrorCode) -> erlang:nif_error(nif_library_not_loaded). +-spec close_registration(reg_handle()) -> ok | {error | badarg}. +close_registration(_Handle) -> + erlang:nif_error(nif_library_not_loaded). + -spec get_registration_name(reg_handle()) -> {ok, string()} | {error, badarg}. get_registration_name(_Handle) -> erlang:nif_error(nif_library_not_loaded). From cb86c485e06f7fbb40e6178158e90ba26968d2ff Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 9 Oct 2023 15:11:25 +0200 Subject: [PATCH 14/22] refactor: closing msquic handles early, part 2 --- c_src/quicer_connection.c | 38 ++++++++++++++++++++------------------ c_src/quicer_ctx.c | 5 ++++- c_src/quicer_nif.c | 5 +++++ c_src/quicer_stream.c | 6 ++++-- 4 files changed, 33 insertions(+), 21 deletions(-) diff --git a/c_src/quicer_connection.c b/c_src/quicer_connection.c index 1a80b3bd..8ccfe362 100644 --- a/c_src/quicer_connection.c +++ b/c_src/quicer_connection.c @@ -327,7 +327,6 @@ _IRQL_requires_max_(DISPATCH_LEVEL) // @see async_connect3 status = handle_connection_event_shutdown_complete(c_ctx, Event); is_destroy = TRUE; - c_ctx->is_closed = TRUE; // client shutdown completed break; case QUIC_CONNECTION_EVENT_PEER_ADDRESS_CHANGED: @@ -372,17 +371,22 @@ _IRQL_requires_max_(DISPATCH_LEVEL) break; } enif_clear_env(env); - enif_mutex_unlock(c_ctx->lock); + QuicerConfigCTX *conf_ctx = c_ctx->config_resource; if (is_destroy) { - // MsQuic->SetCallbackHandler(Connection, NULL, NULL); + c_ctx->is_closed = TRUE; // client shutdown completed c_ctx->Connection = NULL; + c_ctx->config_resource = NULL; + } + enif_mutex_unlock(c_ctx->lock); + + if (is_destroy) + { MsQuic->ConnectionClose(Connection); - if (c_ctx->config_resource) + if (conf_ctx) { - enif_release_resource(c_ctx->config_resource); - c_ctx->config_resource = NULL; + enif_release_resource(conf_ctx); } destroy_c_ctx(c_ctx); } @@ -442,7 +446,6 @@ ServerConnectionCallback(HQUIC Connection, // safely cleaned up. // status = handle_connection_event_shutdown_complete(c_ctx, Event); - c_ctx->is_closed = TRUE; // server shutdown_complete is_destroy = TRUE; break; case QUIC_CONNECTION_EVENT_PEER_STREAM_STARTED: @@ -489,16 +492,22 @@ ServerConnectionCallback(HQUIC Connection, break; } enif_clear_env(env); - enif_mutex_unlock(c_ctx->lock); + QuicerConfigCTX *conf_ctx = c_ctx->config_resource; if (is_destroy) { c_ctx->Connection = NULL; + c_ctx->is_closed = TRUE; // server shutdown_complete + c_ctx->config_resource = NULL; + } + enif_mutex_unlock(c_ctx->lock); + + if (is_destroy) + { MsQuic->ConnectionClose(Connection); - if (c_ctx->config_resource) + if (conf_ctx) { - enif_release_resource(c_ctx->config_resource); - c_ctx->config_resource = NULL; + enif_release_resource(conf_ctx); } destroy_c_ctx(c_ctx); } @@ -814,13 +823,6 @@ async_connect3(ErlNifEnv *env, AcceptorDestroy(c_ctx->owner); c_ctx->owner = NULL; - /* Although MsQuic internally close the connection after failed to start, - we still do not need to set is_closed here, we expect callback to set - it while handling the shutdown complete event otherwise could cause - race cond. - */ - // c_ctx->is_closed = TRUE; - if (Status != QUIC_STATUS_INVALID_PARAMETER) { c_ctx->Connection = NULL; diff --git a/c_src/quicer_ctx.c b/c_src/quicer_ctx.c index 707af5ce..285e43a8 100644 --- a/c_src/quicer_ctx.c +++ b/c_src/quicer_ctx.c @@ -215,7 +215,10 @@ deinit_config_ctx(QuicerConfigCTX *config_ctx) void destroy_config_ctx(QuicerConfigCTX *config_ctx) { - enif_release_resource(config_ctx); + if (config_ctx) + { + enif_release_resource(config_ctx); + } } QuicerStreamCTX * diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index e99ab381..5141d733 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -887,8 +887,10 @@ resource_conn_down_callback(__unused_parm__ ErlNifEnv *env, && !enif_compare_pids(&c_ctx->owner->Pid, DeadPid)) { TP_CB_3(start, (uintptr_t)c_ctx->Connection, (uintptr_t)ctx); + enif_mutex_lock(c_ctx->lock); MsQuic->ConnectionShutdown( c_ctx->Connection, QUIC_CONNECTION_SHUTDOWN_FLAG_NONE, 0); + enif_mutex_unlock(c_ctx->lock); TP_CB_3(end, (uintptr_t)c_ctx->Connection, (uintptr_t)ctx); } } @@ -901,6 +903,7 @@ resource_stream_dealloc_callback(__unused_parm__ ErlNifEnv *env, void *obj) assert(s_ctx->is_closed == TRUE); if (s_ctx->Stream) { + MsQuic->StreamClose(s_ctx->Stream); } @@ -920,6 +923,7 @@ resource_stream_down_callback(__unused_parm__ ErlNifEnv *env, QUIC_STATUS status = QUIC_STATUS_SUCCESS; QuicerStreamCTX *s_ctx = ctx; + enif_mutex_lock(s_ctx->lock); if (s_ctx && s_ctx->owner && DeadPid && !enif_compare_pids(&s_ctx->owner->Pid, DeadPid)) { @@ -938,6 +942,7 @@ resource_stream_down_callback(__unused_parm__ ErlNifEnv *env, TP_CB_3(shutdown_success, (uintptr_t)s_ctx->Stream, status); } } + enif_mutex_unlock(s_ctx->lock); } void diff --git a/c_src/quicer_stream.c b/c_src/quicer_stream.c index 50b7d758..10fe90fc 100644 --- a/c_src/quicer_stream.c +++ b/c_src/quicer_stream.c @@ -230,6 +230,7 @@ _IRQL_requires_max_(DISPATCH_LEVEL) if (is_destroy) { s_ctx->is_closed = TRUE; + s_ctx->Stream = NULL; MsQuic->SetCallbackHandler(Stream, NULL, NULL); } @@ -238,7 +239,6 @@ _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); } @@ -551,6 +551,7 @@ csend4(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) { res = ERROR_TUPLE_3(ATOM_STREAM_OPEN_ERROR, ATOM_STATUS(Status)); + s_ctx->Stream = NULL; goto ErrorExit; } @@ -822,12 +823,13 @@ shutdown_stream3(ErlNifEnv *env, ret = ERROR_TUPLE_2(ATOM_BADARG); } + enif_mutex_lock(s_ctx->lock); if (QUIC_FAILED(Status = MsQuic->StreamShutdown(s_ctx->Stream, flags, app_errcode))) { ret = ERROR_TUPLE_2(ATOM_STATUS(Status)); } - + enif_mutex_unlock(s_ctx->lock); return ret; } From 25e4a359780b2537af72a544fb7fd7d633dd7f00 Mon Sep 17 00:00:00 2001 From: William Yang Date: Mon, 9 Oct 2023 15:33:58 +0200 Subject: [PATCH 15/22] fix: flaky test --- test/quicer_SUITE.erl | 28 +++++++++++++++++++++------- test/quicer_connection_SUITE.erl | 3 ++- test/quicer_snb_SUITE.erl | 30 ++++++++++++++---------------- 3 files changed, 37 insertions(+), 24 deletions(-) diff --git a/test/quicer_SUITE.erl b/test/quicer_SUITE.erl index 98c2fe29..0070dfb1 100644 --- a/test/quicer_SUITE.erl +++ b/test/quicer_SUITE.erl @@ -582,7 +582,11 @@ tc_stream_passive_receive_shutdown(Config) -> {ok, <<"pong">>} = quicer:recv(Stm, 0), {ok, 4} = quicer:send(Stm, <<"ping">>, ?QUIC_SEND_FLAG_FIN), {ok, <<"pong">>} = quicer:recv(Stm, 0), - {error, peer_send_shutdown} = quicer:recv(Stm, 0), + case quicer:recv(Stm, 0) of + {error, peer_send_shutdown} -> ok; + {error, invalid_parameter} -> ok; + {error, closed} -> ok + end, quicer:close_connection(Conn), SPid ! done, ensure_server_exit_normal(Ref) @@ -620,7 +624,10 @@ tc_stream_passive_receive_aborted(Config) -> {ok, 4} = quicer:send(Stm, <<"ping">>), {ok, <<"ping">>} = quicer:recv(Stm, 0), {ok, 5} = quicer:send(Stm, <<"Abort">>), - {error, peer_send_aborted} = quicer:recv(Stm, 0), + case quicer:recv(Stm, 0) of + {error, peer_send_aborted} -> ok; + {error, invalid_parameter} -> ok + end, quicer:close_connection(Conn), SPid ! done, ensure_server_exit_normal(Ref) @@ -2160,9 +2167,13 @@ tc_event_start_compl_client(Config) -> [{param_conn_disable_1rtt_encryption, true} | default_conn_opts()], 5000), %% Stream 1 enabled - {ok, Stm} = quicer:start_stream(Conn, [{active, true}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE}]), + {ok, Stm} = quicer:start_stream(Conn, [{active, true}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE}, + {start_flag, ?QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL} + ]), %% Stream 2 disabled - {ok, Stm2} = quicer:start_stream(Conn, [{active, true}, {quic_event_mask, 0}]), + {ok, Stm2} = quicer:start_stream(Conn, [{active, true}, {quic_event_mask, 0}, + {start_flag, ?QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL} + ]), {ok, 5} = quicer:async_send(Stm, <<"ping1">>), {ok, 5} = quicer:async_send(Stm, <<"ping2">>), receive @@ -2177,7 +2188,7 @@ tc_event_start_compl_client(Config) -> {quic, start_completed, Stm2, #{status := Status}} -> ct:fail("Stream ~p should NOT recv event : ~p", [Stm, Status]) - after 0 -> + after 500 -> ok end, quicer:close_connection(Conn), @@ -2355,6 +2366,7 @@ tc_direct_send_over_conn_block(Config) -> after 100 -> ct:pal("No resp from unidi Stm2") end, + quicer:close_connection(Conn), ok. tc_direct_send_over_conn_fail(Config) -> @@ -2374,14 +2386,16 @@ tc_direct_send_over_conn_fail(Config) -> [{param_conn_disable_1rtt_encryption, true} | default_conn_opts()]), %% Stream 1 enabled - {ok, Stm} = quicer:start_stream(Conn, [{active, true}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE}]), + {ok, Stm} = quicer:start_stream(Conn, [{active, true}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE}, + {start_flag, ?QUIC_STREAM_START_FLAG_SHUTDOWN_ON_FAIL} + ]), {ok, 5} = quicer:async_send(Stm, <<"ping1">>), quicer:shutdown_connection(Conn), %% csend over a closed conn - case quicer:async_csend(Conn, <<"ping2">>, [{active, true}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE}, + case quicer:async_csend(Conn, <<"ping22">>, [{active, true}, {quic_event_mask, ?QUICER_STREAM_EVENT_MASK_START_COMPLETE}, {open_flag, ?QUIC_STREAM_OPEN_FLAG_UNIDIRECTIONAL} ], ?QUIC_SEND_FLAG_ALLOW_0_RTT) of {error, closed} -> ok; diff --git a/test/quicer_connection_SUITE.erl b/test/quicer_connection_SUITE.erl index fbe31a95..f4d6d610 100644 --- a/test/quicer_connection_SUITE.erl +++ b/test/quicer_connection_SUITE.erl @@ -486,7 +486,8 @@ run_tc_conn_client_bad_cert(Config)-> case quicer:send(Stm, <<"ping">>) of {ok, 4} -> ok; {error, cancelled} -> ok; - {error, stm_send_error, aborted} -> ok + {error, stm_send_error, aborted} -> ok; + {error, closed} -> ok end, receive {quic, transport_shutdown, _Ref, diff --git a/test/quicer_snb_SUITE.erl b/test/quicer_snb_SUITE.erl index a08b0e14..a75479bc 100644 --- a/test/quicer_snb_SUITE.erl +++ b/test/quicer_snb_SUITE.erl @@ -947,7 +947,19 @@ tc_conn_gc(Config) -> timer:sleep(1000), {SRid, CRid} end, - fun({_SRid, CRid}, Trace) -> + fun({_SRid, CRid}, Trace0) -> + Trace = flush_previous_run(Trace0, fun(#{ ?snk_kind := debug + , context := "callback" + , function := "ClientConnectionCallback" + , mark := ?QUIC_CONNECTION_EVENT_CONNECTED + , resource_id := Rid + , tag := "event" + }) when Rid == CRid -> + true; + (_) -> + false + end + ), ct:pal("Trace is ~p", [Trace]), ct:pal("Target SRid: ~p, CRid: ~p", [_SRid, CRid]), %% check that at client side, GC is triggered after connection close. @@ -979,23 +991,9 @@ tc_conn_gc(Config) -> , mark := ?QUIC_CONNECTION_EVENT_SHUTDOWN_COMPLETE , tag := "event"}, Trace)), - - - TraceEvents = flush_previous_run(Trace, fun(#{ ?snk_kind := debug - , context := "callback" - , function := "ClientConnectionCallback" - , mark := ?QUIC_CONNECTION_EVENT_CONNECTED - , resource_id := Rid - , tag := "event" - }) when Rid == CRid -> - true; - (_) -> - false - end - ), ?assertEqual(1, length([ E || #{ function := "resource_conn_dealloc_callback" , resource_id := Rid - , tag := "end"} = E <- TraceEvents, Rid == 0]) + , tag := "end"} = E <- Trace, Rid == 0]) ) end), ct:pal("stop listener"), From 5d2aa518009b88ff98894e3e4a0aadaf08fc69cd Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 10 Oct 2023 09:23:52 +0200 Subject: [PATCH 16/22] doc: update QUIC_TRACE_API --- docs/internals.org | 56 ++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/docs/internals.org b/docs/internals.org index 3bdcfc8b..134604a0 100644 --- a/docs/internals.org +++ b/docs/internals.org @@ -250,33 +250,35 @@ SYNC call in non-callback-context *** API Types (number in tracing) #+begin_verse -QUIC_TRACE_API_SET_PARAM, // 0 -QUIC_TRACE_API_GET_PARAM, // 1 -QUIC_TRACE_API_REGISTRATION_OPEN , // 2 -QUIC_TRACE_API_REGISTRATION_CLOSE, // 3 -QUIC_TRACE_API_REGISTRATION_SHUTDOWN, // 4 -QUIC_TRACE_API_CONFIGURATION_OPEN, // 5 -QUIC_TRACE_API_CONFIGURATION_CLOSE, // 6 -QUIC_TRACE_API_CONFIGURATION_LOAD_CREDENTIAL, // 7 -QUIC_TRACE_API_LISTENER_OPEN, // 8 -QUIC_TRACE_API_LISTENER_CLOSE, // 9 -QUIC_TRACE_API_LISTENER_START, // 10 -QUIC_TRACE_API_LISTENER_STOP, // 11 -QUIC_TRACE_API_CONNECTION_OPEN, // 12 -QUIC_TRACE_API_CONNECTION_CLOSE, // 13 -QUIC_TRACE_API_CONNECTION_SHUTDOWN, // 14 -QUIC_TRACE_API_CONNECTION_START, // 15 -QUIC_TRACE_API_CONNECTION_SET_CONFIGURATION, // 16 -QUIC_TRACE_API_CONNECTION_SEND_RESUMPTION_TICKET, // 17 -QUIC_TRACE_API_STREAM_OPEN, // 18 -QUIC_TRACE_API_STREAM_CLOSE, // 19 -QUIC_TRACE_API_STREAM_START, // 20 -QUIC_TRACE_API_STREAM_SHUTDOWN, // 21 -QUIC_TRACE_API_STREAM_SEND, // 22 -QUIC_TRACE_API_STREAM_RECEIVE_COMPLETE, // 23 -QUIC_TRACE_API_STREAM_RECEIVE_SET_ENABLED, // 24 -QUIC_TRACE_API_DATAGRAM_SEND, // 25 -QUIC_TRACE_API_COUNT // 26 +0 QUIC_TRACE_API_SET_PARAM, +1 QUIC_TRACE_API_GET_PARAM, +2 QUIC_TRACE_API_REGISTRATION_OPEN, +3 QUIC_TRACE_API_REGISTRATION_CLOSE, +4 QUIC_TRACE_API_REGISTRATION_SHUTDOWN, +5 QUIC_TRACE_API_CONFIGURATION_OPEN, +6 QUIC_TRACE_API_CONFIGURATION_CLOSE, +7 QUIC_TRACE_API_CONFIGURATION_LOAD_CREDENTIAL, +8 QUIC_TRACE_API_LISTENER_OPEN, +9 QUIC_TRACE_API_LISTENER_CLOSE, +10 QUIC_TRACE_API_LISTENER_START, +11 QUIC_TRACE_API_LISTENER_STOP, +12 QUIC_TRACE_API_CONNECTION_OPEN, +13 QUIC_TRACE_API_CONNECTION_CLOSE, +14 QUIC_TRACE_API_CONNECTION_SHUTDOWN, +15 QUIC_TRACE_API_CONNECTION_START, +16 QUIC_TRACE_API_CONNECTION_SET_CONFIGURATION, +17 QUIC_TRACE_API_CONNECTION_SEND_RESUMPTION_TICKET, +18 QUIC_TRACE_API_CONNECTION_COMPLETE_RESUMPTION_TICKET_VALIDATION, +19 QUIC_TRACE_API_CONNECTION_COMPLETE_CERTIFICATE_VALIDATION, +20 QUIC_TRACE_API_STREAM_OPEN, +21 QUIC_TRACE_API_STREAM_CLOSE, +22 QUIC_TRACE_API_STREAM_START, +23 QUIC_TRACE_API_STREAM_SHUTDOWN, +24 QUIC_TRACE_API_STREAM_SEND, +25 QUIC_TRACE_API_STREAM_RECEIVE_COMPLETE, +26 QUIC_TRACE_API_STREAM_RECEIVE_SET_ENABLED, +27 QUIC_TRACE_API_DATAGRAM_SEND, +28 QUIC_TRACE_API_COUNT // Must be last #+end_verse * Event handling From 81f6037858b4ba7cd97d7d76ea72f5adff2d5293 Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 10 Oct 2023 11:36:09 +0200 Subject: [PATCH 17/22] fix: close msquic stream handle if start stream is failed msquic API Doc: StreamOpen.md So, apps that rely on that event to trigger clean up of the stream **must** handle the case where [StreamStart](StreamStart.md) is either not ever called or fails and clean up directly. --- c_src/quicer_stream.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/c_src/quicer_stream.c b/c_src/quicer_stream.c index 10fe90fc..41d82505 100644 --- a/c_src/quicer_stream.c +++ b/c_src/quicer_stream.c @@ -354,8 +354,11 @@ async_start_stream2(ErlNifEnv *env, // if (QUIC_FAILED(Status = MsQuic->StreamStart(s_ctx->Stream, start_flag))) { - // note, stream call back would close the stream - // destroy_s_ctx should not be called here + HQUIC Stream = s_ctx->Stream; + enif_mutex_lock(s_ctx->lock); + s_ctx->is_closed = TRUE; + enif_mutex_unlock(s_ctx->lock); + MsQuic->StreamClose(Stream); return ERROR_TUPLE_3(ATOM_STREAM_START_ERROR, ATOM_STATUS(Status)); } // NOTE: Set is_closed to FALSE (s_ctx->is_closed = FALSE;) From 39e30db24a4df6eeac84f6c1557d760474f652d1 Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 10 Oct 2023 16:08:34 +0200 Subject: [PATCH 18/22] chore: cleanup --- c_src/quicer_config.c | 5 +++-- c_src/quicer_nif.c | 10 ++++++---- c_src/quicer_vsn.h.in | 15 +++++++++++++++ include/quicer_vsn.hrl.in | 15 +++++++++++++++ test/quicer_snb_SUITE.erl | 2 +- 5 files changed, 40 insertions(+), 7 deletions(-) diff --git a/c_src/quicer_config.c b/c_src/quicer_config.c index d04739a2..e720dc94 100644 --- a/c_src/quicer_config.c +++ b/c_src/quicer_config.c @@ -770,8 +770,9 @@ getopt3(ErlNifEnv *env, if (IS_SAME_TERM(ATOM_QUIC_GLOBAL, ctx)) { pthread_mutex_lock(&MsQuicLock); - // Get global option without using any ctx - // We are risking using a closed lib + // In a env that while there is no allocated NIF resources (reg, conf, + // listener, conn, stream), VM may unload the module causes unloading DSO + // in parallel. if (MsQuic) { res = get_global_opt(env, NULL, eopt); diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index 5141d733..3782b2df 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -803,13 +803,16 @@ resource_listener_down_callback(__unused_parm__ ErlNifEnv *env, if (!l_ctx->is_closed && !l_ctx->is_stopped && l_ctx->Listener) { 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 + // handle. // b. Some pid could still close the stopped listener with nif - // handle - // c. We close it in resource_listener_dealloc_callback anyway. + // handle. + // c. We close it in resource_listener_dealloc_callback anyway when + // Listener term get GC. + */ MsQuic->ListenerStop(l_ctx->Listener); } enif_mutex_unlock(l_ctx->lock); @@ -903,7 +906,6 @@ resource_stream_dealloc_callback(__unused_parm__ ErlNifEnv *env, void *obj) assert(s_ctx->is_closed == TRUE); if (s_ctx->Stream) { - MsQuic->StreamClose(s_ctx->Stream); } diff --git a/c_src/quicer_vsn.h.in b/c_src/quicer_vsn.h.in index acc3a6a9..ae953305 100644 --- a/c_src/quicer_vsn.h.in +++ b/c_src/quicer_vsn.h.in @@ -1,3 +1,18 @@ +/*-------------------------------------------------------------------- +Copyright (c) 2023 EMQ Technologies Co., Ltd. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +-------------------------------------------------------------------*/ #ifndef __QUICER_VSN_H_ #define __QUICER_VSN_H_ diff --git a/include/quicer_vsn.hrl.in b/include/quicer_vsn.hrl.in index 02d0b4e1..db73c6ad 100644 --- a/include/quicer_vsn.hrl.in +++ b/include/quicer_vsn.hrl.in @@ -1,3 +1,18 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- -ifndef(QUICER_VSN_HRL). -define(QUICER_VSN_HRL, true). -define(QUICER_ABI_VERSION, @QUICER_ABI_VERSION@). diff --git a/test/quicer_snb_SUITE.erl b/test/quicer_snb_SUITE.erl index a75479bc..01fb329e 100644 --- a/test/quicer_snb_SUITE.erl +++ b/test/quicer_snb_SUITE.erl @@ -1023,13 +1023,13 @@ tc_conn_no_gc(Config) -> {ok, Conn} = quicer:connect("localhost", Port, [{idle_timeout_ms, 1000}, {verify, none}, {alpn, ["sample"]}], 5000), + {ok, CRid} = quicer:get_conn_rid(Conn), _Child = spawn_link(fun() -> {ok, Stm} = quicer:start_stream(Conn, [{active, false}]), {ok, 4} = quicer:async_send(Stm, <<"ping">>), {ok, <<"ping">>} = quicer:recv(Stm, 4), quicer:shutdown_connection(Conn, 0, 0) end), - {ok, CRid} = quicer:get_conn_rid(Conn), %% Server Process {ok, #{resource_id := SRid}} = ?block_until(#{ ?snk_kind := debug From 93686bee8f5c64992fa574a224a21d7c4bdb3596 Mon Sep 17 00:00:00 2001 From: William Yang Date: Tue, 10 Oct 2023 17:10:15 +0200 Subject: [PATCH 19/22] chore: run close_registration on dirty scheduler --- c_src/quicer_nif.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c_src/quicer_nif.c b/c_src/quicer_nif.c index 3782b2df..60876849 100644 --- a/c_src/quicer_nif.c +++ b/c_src/quicer_nif.c @@ -1535,7 +1535,7 @@ static ErlNifFunc nif_funcs[] = { { "new_registration", 2, new_registration2, 0}, { "shutdown_registration", 1, shutdown_registration_x, 0}, { "shutdown_registration", 3, shutdown_registration_x, 0}, - { "close_registration", 1, close_registration, 0}, + { "close_registration", 1, close_registration, 1}, { "get_registration_name", 1, get_registration_name1, 0}, { "listen", 2, listen2, 0}, { "start_listener", 3, start_listener3, 0}, From dc724312707ff15b798405370dd194dadf3dad50 Mon Sep 17 00:00:00 2001 From: William Yang Date: Wed, 11 Oct 2023 13:58:55 +0200 Subject: [PATCH 20/22] fix: patch msquic 2.2 memleak patch: https://github.com/microsoft/msquic/commit/73a11d7bdc724432964a2d4bdc4211ed29823380.patch --- get-msquic.sh | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/get-msquic.sh b/get-msquic.sh index 63c38c00..a6327bb6 100755 --- a/get-msquic.sh +++ b/get-msquic.sh @@ -4,6 +4,29 @@ set -euo pipefail VERSION="$1" +patch_dir="patches" + +do_patch() +{ + patch_source="$1" + patch_file="${patch_dir}/$(basename ${patch_source})" + curl -f -L -o "${patch_file}" "$patch_source" + if patch -p1 -f --dry-run -s < "${patch_file}" 2>/dev/null; then + patch -p1 < "${patch_file}" + else + echo "Skip patching ${patch_file}, already applied" + fi +} + +patch_2_2_3() +{ + local patch_1="https://github.com/microsoft/msquic/commit/73a11d7bdc724432964a2d4bdc4211ed29823380.patch" + mkdir -p "$patch_dir" + echo "Patching Msquic 2.2.3" + do_patch "$patch_1" +} + + if [ ! -d msquic ]; then git clone https://github.com/microsoft/msquic.git -b "$VERSION" --recursive --depth 1 --shallow-submodules msquic fi @@ -20,3 +43,9 @@ if [ "$CURRENT_VSN" != "$VERSION" ]; then echo "undesired_msquic_version, required=$VERSION, got=$CURRENT_VSN" exit 1 fi + +## Patching +case $VERSION in + v2.2.3) + patch_2_2_3 +esac From 702fac49fa757f03ad300730ebe423fdc89778a8 Mon Sep 17 00:00:00 2001 From: William Yang Date: Wed, 11 Oct 2023 16:08:30 +0200 Subject: [PATCH 21/22] fix: check handle not NULL in quicer:recv with mutex It is just nice to call it to avoid unnecessary return '{error, invalid_parameter}' --- c_src/quicer_stream.c | 6 ++++++ test/quicer_SUITE.erl | 3 +-- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/c_src/quicer_stream.c b/c_src/quicer_stream.c index 41d82505..3d59087f 100644 --- a/c_src/quicer_stream.c +++ b/c_src/quicer_stream.c @@ -736,6 +736,12 @@ recv2(ErlNifEnv *env, __unused_parm__ int argc, const ERL_NIF_TERM argv[]) TP_NIF_3(start, (uintptr_t)s_ctx->Stream, size_req); enif_mutex_lock(s_ctx->lock); + if ( !s_ctx->Stream ) + { + res = ERROR_TUPLE_2(ATOM_CLOSED); + goto Exit; + } + if (ACCEPTOR_RECV_MODE_PASSIVE != s_ctx->owner->active) { res = ERROR_TUPLE_2(ATOM_EINVAL); diff --git a/test/quicer_SUITE.erl b/test/quicer_SUITE.erl index 0070dfb1..9830c0cb 100644 --- a/test/quicer_SUITE.erl +++ b/test/quicer_SUITE.erl @@ -584,7 +584,6 @@ tc_stream_passive_receive_shutdown(Config) -> {ok, <<"pong">>} = quicer:recv(Stm, 0), case quicer:recv(Stm, 0) of {error, peer_send_shutdown} -> ok; - {error, invalid_parameter} -> ok; {error, closed} -> ok end, quicer:close_connection(Conn), @@ -626,7 +625,7 @@ tc_stream_passive_receive_aborted(Config) -> {ok, 5} = quicer:send(Stm, <<"Abort">>), case quicer:recv(Stm, 0) of {error, peer_send_aborted} -> ok; - {error, invalid_parameter} -> ok + {error, closed} -> ok end, quicer:close_connection(Conn), SPid ! done, From 08db52b7e6740c9299763f40bdaa291ad5456fa7 Mon Sep 17 00:00:00 2001 From: William Yang Date: Wed, 11 Oct 2023 16:10:39 +0200 Subject: [PATCH 22/22] fix: get right arg when open connection --- c_src/quicer_connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/c_src/quicer_connection.c b/c_src/quicer_connection.c index 8ccfe362..2a5fb2fd 100644 --- a/c_src/quicer_connection.c +++ b/c_src/quicer_connection.c @@ -527,7 +527,7 @@ open_connectionX(ErlNifEnv *env, int argc, const ERL_NIF_TERM argv[]) ERL_NIF_TERM res = ERROR_TUPLE_2(ATOM_ERROR_INTERNAL_ERROR); QuicerRegistrationCTX *r_ctx = NULL; HQUIC registration = NULL; - ERL_NIF_TERM options = argv[1]; + ERL_NIF_TERM options = argv[0]; if (argc == 0) {