From d4458618b2339638274726663a39ec96d28a4146 Mon Sep 17 00:00:00 2001
From: k32 <10274441+k32@users.noreply.github.com>
Date: Mon, 21 Feb 2022 13:40:06 +0100
Subject: [PATCH] fix(hook): Executed hooks in deterministic order

---
 apps/emqx/src/emqx_hooks.erl        |  5 +++-
 apps/emqx/test/emqx_SUITE.erl       |  2 +-
 apps/emqx/test/emqx_hooks_SUITE.erl | 42 +++++++++++++++++++++++------
 3 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/apps/emqx/src/emqx_hooks.erl b/apps/emqx/src/emqx_hooks.erl
index 0b81cbee0..2cc5a1740 100644
--- a/apps/emqx/src/emqx_hooks.erl
+++ b/apps/emqx/src/emqx_hooks.erl
@@ -296,7 +296,10 @@ add_callback(C, Callbacks) ->
 add_callback(C, [], Acc) ->
     lists:reverse([C | Acc]);
 add_callback(C1 = #callback{priority = P1}, [C2 = #callback{priority = P2} | More], Acc)
-    when P1 =< P2 ->
+    when P1 < P2 ->
+    add_callback(C1, More, [C2 | Acc]);
+add_callback(C1 = #callback{priority = P1, action = MFA1}, [C2 = #callback{priority = P2, action = MFA2} | More], Acc)
+    when P1 =:= P2 andalso MFA1 >= MFA2 ->
     add_callback(C1, More, [C2 | Acc]);
 add_callback(C1, More, Acc) ->
     lists:append(lists:reverse(Acc), [C1 | More]).
diff --git a/apps/emqx/test/emqx_SUITE.erl b/apps/emqx/test/emqx_SUITE.erl
index 0420e3b02..8ef94b4f6 100644
--- a/apps/emqx/test/emqx_SUITE.erl
+++ b/apps/emqx/test/emqx_SUITE.erl
@@ -115,7 +115,7 @@ t_run_hook(_) ->
 
     ok = emqx:hook(foldl_hook2, {?MODULE, hook_fun9, []}),
     ok = emqx:hook(foldl_hook2, {?MODULE, hook_fun10, []}),
-    [r9] = emqx:run_fold_hook(foldl_hook2, [arg], []),
+    [r10] = emqx:run_fold_hook(foldl_hook2, [arg], []),
 
     ok = emqx:hook(foreach_hook, {?MODULE, hook_fun6, [initArg]}),
     {error, already_exists} = emqx:hook(foreach_hook, {?MODULE, hook_fun6, [initArg]}),
diff --git a/apps/emqx/test/emqx_hooks_SUITE.erl b/apps/emqx/test/emqx_hooks_SUITE.erl
index 355553c91..eb0189b42 100644
--- a/apps/emqx/test/emqx_hooks_SUITE.erl
+++ b/apps/emqx/test/emqx_hooks_SUITE.erl
@@ -19,10 +19,17 @@
 -compile(export_all).
 -compile(nowarn_export_all).
 
+-include_lib("proper/include/proper.hrl").
 -include_lib("eunit/include/eunit.hrl").
 
 all() -> emqx_common_test_helpers:all(?MODULE).
 
+init_per_testcase(_, Config) ->
+    Config.
+
+end_per_testcase(_) ->
+    catch emqx_hooks:stop().
+
 % t_lookup(_) ->
 %     error('TODO').
 
@@ -37,7 +44,30 @@ all() -> emqx_common_test_helpers:all(?MODULE).
 
 % t_add(_) ->
 %     error('TODO').
-    
+
+t_add_hook_order(_) ->
+    ?assert(proper:quickcheck(add_hook_order_prop(), [{on_output, fun ct:print/2}])).
+
+add_hook_order_prop() ->
+    %% Note: order is inversed, since higher prio hooks run first:
+    Comparator = fun({Prio1, M1, F1}, {Prio2, M2, F2}) ->
+                         Prio1 > Prio2 orelse
+                             (Prio1 =:= Prio2 andalso {M1, F1} =< {M2, F2})
+                 end,
+    ?FORALL(
+       Hooks, list({range(-1, 5), atom(), atom()}),
+       try
+           {ok, _} = emqx_hooks:start_link(),
+           [emqx:hook(prop_hook, {M, F, []}, Prio) || {Prio, M, F} <- Hooks],
+           Callbacks = emqx_hooks:lookup(prop_hook),
+           Order = [{Prio, M, F} || {callback, {M, F, _}, _Filter, Prio} <- Callbacks],
+           ?assertEqual(lists:sort(Comparator, Hooks),
+                        Order),
+           true
+       after
+           emqx_hooks:stop()
+       end).
+
 t_add_put_del_hook(_) ->
     {ok, _} = emqx_hooks:start_link(),
     ok = emqx:hook(test_hook, {?MODULE, hook_fun1, []}),
@@ -84,8 +114,7 @@ t_add_put_del_hook(_) ->
     ok = emqx:unhook(emqx_hook, {?MODULE, hook_fun9, [test]}),
     ok = emqx:unhook(emqx_hook, {?MODULE, hook_fun10, [test]}),
     timer:sleep(200),
-    ?assertEqual([], emqx_hooks:lookup(emqx_hook)),
-    ok = emqx_hooks:stop().
+    ?assertEqual([], emqx_hooks:lookup(emqx_hook)).
 
 t_run_hooks(_) ->
     {ok, _} = emqx_hooks:start_link(),
@@ -97,7 +126,7 @@ t_run_hooks(_) ->
 
     ok = emqx:hook(foldl_hook2, {?MODULE, hook_fun9, []}),
     ok = emqx:hook(foldl_hook2, {?MODULE, hook_fun10, []}),
-    [r9] = emqx:run_fold_hook(foldl_hook2, [arg], []),
+    [r10] = emqx:run_fold_hook(foldl_hook2, [arg], []), %% Note: 10 is _less_ than 9 per lexicographic order
 
     ok = emqx:hook(foreach_hook, {?MODULE, hook_fun6, [initArg]}),
     {error, already_exists} = emqx:hook(foreach_hook, {?MODULE, hook_fun6, [initArg]}),
@@ -112,9 +141,7 @@ t_run_hooks(_) ->
     ok = emqx:hook(foldl_filter2_hook, {?MODULE, hook_fun2, []}, {?MODULE, hook_filter2, [init_arg]}),
     ok = emqx:hook(foldl_filter2_hook, {?MODULE, hook_fun2_1, []}, {?MODULE, hook_filter2_1, [init_arg]}),
     ?assertEqual(3, emqx:run_fold_hook(foldl_filter2_hook, [arg], 1)),
-    ?assertEqual(2, emqx:run_fold_hook(foldl_filter2_hook, [arg1], 1)),
-
-    ok = emqx_hooks:stop().
+    ?assertEqual(2, emqx:run_fold_hook(foldl_filter2_hook, [arg1], 1)).
 
 t_uncovered_func(_) ->
     {ok, _} = emqx_hooks:start_link(),
@@ -157,4 +184,3 @@ hook_filter2(_, _Acc, _IntArg) -> false.
 hook_filter2_1(arg, _Acc, init_arg)  -> true;
 hook_filter2_1(arg1, _Acc, init_arg) -> true;
 hook_filter2_1(_, _Acc, _IntArg)     -> false.
-