From ebb2824e1516fa6aa33645c4de92981d10b2b620 Mon Sep 17 00:00:00 2001
From: JianBo He <heeejianbo@gmail.com>
Date: Fri, 2 Sep 2022 10:26:14 +0800
Subject: [PATCH] test: ensure hooks has unloaded if grpc is blocked

---
 apps/emqx_exhook/src/emqx_exhook_mgr.erl      |  4 ++-
 apps/emqx_exhook/src/emqx_exhook_server.erl   |  6 ++--
 apps/emqx_exhook/src/emqx_exhook_sup.erl      |  1 +
 apps/emqx_exhook/test/emqx_exhook_SUITE.erl   | 35 +++++++++++++++++++
 .../emqx_exhook/test/emqx_exhook_demo_svr.erl |  5 ++-
 .../src/bhvrs/emqx_gateway_conn.erl           |  1 -
 6 files changed, 46 insertions(+), 6 deletions(-)

diff --git a/apps/emqx_exhook/src/emqx_exhook_mgr.erl b/apps/emqx_exhook/src/emqx_exhook_mgr.erl
index 9b062e914..e58555ca1 100644
--- a/apps/emqx_exhook/src/emqx_exhook_mgr.erl
+++ b/apps/emqx_exhook/src/emqx_exhook_mgr.erl
@@ -21,6 +21,7 @@
 
 -include("emqx_exhook.hrl").
 -include_lib("emqx/include/logger.hrl").
+-include_lib("snabbkaffe/include/snabbkaffe.hrl").
 
 %% APIs
 -export([start_link/0]).
@@ -297,7 +298,7 @@ handle_info(refresh_tick, State) ->
 handle_info(_Info, State) ->
     {noreply, State}.
 
-terminate(_Reason, State = #{servers := Servers}) ->
+terminate(Reason, State = #{servers := Servers}) ->
     _ = unload_exhooks(),
     _ = maps:fold(
         fun(Name, _, AccIn) ->
@@ -306,6 +307,7 @@ terminate(_Reason, State = #{servers := Servers}) ->
         State,
         Servers
     ),
+    ?tp(info, exhook_mgr_terminated, #{reason => Reason, servers => Servers}),
     ok.
 
 code_change(_OldVsn, State, _Extra) ->
diff --git a/apps/emqx_exhook/src/emqx_exhook_server.erl b/apps/emqx_exhook/src/emqx_exhook_server.erl
index e5075bce4..311cff316 100644
--- a/apps/emqx_exhook/src/emqx_exhook_server.erl
+++ b/apps/emqx_exhook/src/emqx_exhook_server.erl
@@ -185,9 +185,9 @@ unload(#{name := Name, options := ReqOpts, hookspec := HookSpecs}) ->
     ok.
 
 do_deinit(Name, ReqOpts) ->
-    %% Using shorter timeout to deinit grpc server to avoid emqx_exhook_mgr
-    %% force killed by upper supervisor
-    _ = do_call(Name, undefined, 'on_provider_unloaded', #{}, ReqOpts#{timeout => 3000}),
+    %% Override the request timeout to deinit grpc server to
+    %% avoid emqx_exhook_mgr force killed by upper supervisor
+    _ = do_call(Name, undefined, 'on_provider_unloaded', #{}, ReqOpts#{timeout => 5000}),
     ok.
 
 do_init(ChannName, ReqOpts) ->
diff --git a/apps/emqx_exhook/src/emqx_exhook_sup.erl b/apps/emqx_exhook/src/emqx_exhook_sup.erl
index fb424ddff..3423eaf59 100644
--- a/apps/emqx_exhook/src/emqx_exhook_sup.erl
+++ b/apps/emqx_exhook/src/emqx_exhook_sup.erl
@@ -32,6 +32,7 @@
     id => Mod,
     start => {Mod, start_link, Args},
     type => Type,
+    %% long timeout for emqx_exhook_mgr
     shutdown => 15000
 }).
 
diff --git a/apps/emqx_exhook/test/emqx_exhook_SUITE.erl b/apps/emqx_exhook/test/emqx_exhook_SUITE.erl
index 62606cf18..a0472b2c3 100644
--- a/apps/emqx_exhook/test/emqx_exhook_SUITE.erl
+++ b/apps/emqx_exhook/test/emqx_exhook_SUITE.erl
@@ -24,6 +24,7 @@
 -include_lib("eunit/include/eunit.hrl").
 -include_lib("common_test/include/ct.hrl").
 -include_lib("emqx/include/emqx_hooks.hrl").
+-include_lib("snabbkaffe/include/snabbkaffe.hrl").
 
 -define(DEFAULT_CLUSTER_NAME_ATOM, emqxcl).
 
@@ -313,6 +314,40 @@ t_cluster_name(_) ->
     ),
     emqx_exhook_mgr:disable(<<"default">>).
 
+t_stop_timeout(_) ->
+    snabbkaffe:start_trace(),
+    meck:new(emqx_exhook_demo_svr, [passthrough, no_history]),
+    meck:expect(
+        emqx_exhook_demo_svr,
+        on_provider_unloaded,
+        fun(Req, Md) ->
+            %% ensure sleep time greater than emqx_exhook_mgr shutdown timeout
+            timer:sleep(20000),
+            meck:passthrough([Req, Md])
+        end
+    ),
+
+    %% stop application
+    application:stop(emqx_exhook),
+    ?block_until(#{?snk_kind := exhook_mgr_terminated}, 20000),
+
+    %% all exhook hooked point should be unloaded
+    Mods = lists:flatten(
+        lists:map(
+            fun({hook, _, Cbs}) ->
+                lists:map(fun({callback, {M, _, _}, _, _}) -> M end, Cbs)
+            end,
+            ets:tab2list(emqx_hooks)
+        )
+    ),
+    ?assertEqual(false, lists:any(fun(M) -> M == emqx_exhook_handler end, Mods)),
+
+    %% ensure started for other tests
+    emqx_common_test_helpers:start_apps([emqx_exhook]),
+
+    snabbkaffe:stop(),
+    meck:unload(emqx_exhook_demo_svr).
+
 %%--------------------------------------------------------------------
 %% Cases Helpers
 %%--------------------------------------------------------------------
diff --git a/apps/emqx_exhook/test/emqx_exhook_demo_svr.erl b/apps/emqx_exhook/test/emqx_exhook_demo_svr.erl
index ea8398eeb..bf1a42c9a 100644
--- a/apps/emqx_exhook/test/emqx_exhook_demo_svr.erl
+++ b/apps/emqx_exhook/test/emqx_exhook_demo_svr.erl
@@ -80,7 +80,10 @@ stop() ->
 
 stop(Name) ->
     grpc:stop_server(Name),
-    to_atom_name(Name) ! stop.
+    case whereis(to_atom_name(Name)) of
+        undefined -> ok;
+        Pid -> Pid ! stop
+    end.
 
 take() ->
     to_atom_name(?NAME) ! {take, self()},
diff --git a/apps/emqx_gateway/src/bhvrs/emqx_gateway_conn.erl b/apps/emqx_gateway/src/bhvrs/emqx_gateway_conn.erl
index 02d6090b4..786142cdd 100644
--- a/apps/emqx_gateway/src/bhvrs/emqx_gateway_conn.erl
+++ b/apps/emqx_gateway/src/bhvrs/emqx_gateway_conn.erl
@@ -640,7 +640,6 @@ handle_timeout(
     Keepalive,
     State = #state{
         chann_mod = ChannMod,
-        socket = Socket,
         channel = Channel
     }
 ) when