Merge pull request #9933 from zhongwencool/keepalive-undefined

fix: don't crash when connecting via keepalive=0
emqx · Feb 8, 2023 · 6f5e6e7 · 6f5e6e7
2 parents efc409b + 0ed4a2f
commit 6f5e6e7
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 10 deletions.
diff --git a/apps/emqx_management/test/emqx_mgmt_api_SUITE.erl b/apps/emqx_management/test/emqx_mgmt_api_SUITE.erl
@@ -794,23 +794,34 @@ t_keepalive(_Config) ->
     Path = api_path(["clients", ClientId, "keepalive"]),
     {ok, NotFound} = request_api(put, Path, "interval=5", AuthHeader, [#{}]),
     ?assertEqual("{\"message\":\"not_found\",\"code\":112}", NotFound),
-    {ok, C1} = emqtt:start_link(#{username => Username, clientid => ClientId}),
-    {ok, _} = emqtt:connect(C1),
-    {ok, Ok} = request_api(put, Path, "interval=5", AuthHeader, [#{}]),
-    ?assertEqual("{\"code\":0}", Ok),
-    [Pid] = emqx_cm:lookup_channels(list_to_binary(ClientId)),
-    #{conninfo := #{keepalive := Keepalive}} = emqx_connection:info(Pid),
-    ?assertEqual(5, Keepalive),
+    C1 = keepalive_ok(61, 0, Username, ClientId, Path, AuthHeader),
     {ok, Error1} = request_api(put, Path, "interval=-1", AuthHeader, [#{}]),
     {ok, Error2} = request_api(put, Path, "interval=65536", AuthHeader, [#{}]),
     ErrMsg = #{<<"code">> => 102,
                <<"message">> => <<"mqtt3.1.1 specification: keepalive must between 0~65535">>},
     ?assertEqual(ErrMsg, jiffy:decode(Error1, [return_maps])),
     ?assertEqual(Error1, Error2),
     emqtt:disconnect(C1),
+    %% test change keepalive from 0 to 60
+    C2 = keepalive_ok(0, 60, Username, ClientId, Path, AuthHeader),
+    emqtt:disconnect(C2),
+    %% test change keepalive from 60 to 0
+    C3 = keepalive_ok(60, 0, Username, ClientId, Path, AuthHeader),
+    emqtt:disconnect(C3),
     application:stop(emqx_dashboard),
     ok.
 
+keepalive_ok(InitSec, UpdateSec, Username, ClientId, Path, AuthHeader) ->
+    {ok, C1} = emqtt:start_link(#{username => Username, clientid => ClientId, keepalive => InitSec}),
+    {ok, _} = emqtt:connect(C1),
+    Qs = "interval=" ++ integer_to_list(UpdateSec),
+    {ok, Ok} = request_api(put, Path, Qs, AuthHeader, [#{}]),
+    ?assertEqual("{\"code\":0}", Ok),
+    [Pid] = emqx_cm:lookup_channels(list_to_binary(ClientId)),
+    #{conninfo := #{keepalive := Keepalive}} = emqx_connection:info(Pid),
+    ?assertEqual(UpdateSec, Keepalive),
+    C1.
+
 t_status_ok(_Config) ->
     {ok, #{ body := Resp
           , status_code := StatusCode

diff --git a/changes/v4.4.15-en.md b/changes/v4.4.15-en.md
@@ -41,3 +41,5 @@
 - fix sometimes the rules cannot be enabled automatically after emqx is restarted [#9911](https://github.com/emqx/emqx/pull/9911).
 
 - fix the `{badarg,[{ets,lookup,[gproc,{shared, ...` error logs during shutdown [#9919](https://github.com/emqx/emqx/pull/9919).
+
+- Fix crash when updating a client's keepalive via the HTTP API if it connects with keepalive disabled [#9933](https://github.com/emqx/emqx/pull/9933).
diff --git a/changes/v4.4.15-zh.md b/changes/v4.4.15-zh.md
@@ -40,3 +40,5 @@
 - 修复某些情况下，重启 emqx 后规则无法自动启用的问题 [#9911](https://github.com/emqx/emqx/pull/9911)。
 
 - 修复停止 emqx 的时候，日志出现 `{badarg,[{ets,lookup,[gproc,{shared, ...` 错误的问题 [#9919](https://github.com/emqx/emqx/pull/9919)。
+
+- 修复当客户端连接禁用 keepalive时, 通过 HTTP API 更新其 keepalive 会崩溃的问题 [#9933](https://github.com/emqx/emqx/pull/9933)。
diff --git a/src/emqx.appup.src b/src/emqx.appup.src
@@ -3,6 +3,7 @@
 {VSN,
   [{"4.4.14",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_misc,brutal_purge,soft_purge,[]},
@@ -15,6 +16,7 @@
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]}]},
    {"4.4.13",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_misc,brutal_purge,soft_purge,[]},
@@ -28,6 +30,7 @@
      {load_module,emqx_app,brutal_purge,soft_purge,[]}]},
    {"4.4.12",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_misc,brutal_purge,soft_purge,[]},
@@ -41,6 +44,7 @@
      {load_module,emqx_app,brutal_purge,soft_purge,[]}]},
    {"4.4.11",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -57,6 +61,7 @@
      {load_module,emqx_channel,brutal_purge,soft_purge,[]}]},
    {"4.4.10",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -82,6 +87,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.9",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -113,6 +119,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.8",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -145,6 +152,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.7",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -177,6 +185,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.6",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -209,6 +218,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.5",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -243,6 +253,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.4",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {load_module,emqx_packet,brutal_purge,soft_purge,[]},
@@ -283,6 +294,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.3",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {add_module,emqx_cover},
@@ -329,6 +341,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.2",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {add_module,emqx_cover},
@@ -376,6 +389,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.1",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {add_module,emqx_cover},
      {add_module,emqx_ocsp_cache},
@@ -427,6 +441,7 @@
                          [gen_rpc,insecure_auth_fallback_allowed,true]}}]},
    {"4.4.0",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {add_module,emqx_cover},
      {add_module,emqx_ocsp_cache},
@@ -481,6 +496,7 @@
    {<<".*">>,[]}],
   [{"4.4.14",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_misc,brutal_purge,soft_purge,[]},
@@ -493,6 +509,7 @@
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]}]},
    {"4.4.13",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_misc,brutal_purge,soft_purge,[]},
@@ -506,6 +523,7 @@
      {load_module,emqx_app,brutal_purge,soft_purge,[]}]},
    {"4.4.12",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_misc,brutal_purge,soft_purge,[]},
@@ -519,6 +537,7 @@
      {load_module,emqx_app,brutal_purge,soft_purge,[]}]},
    {"4.4.11",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -535,6 +554,7 @@
      {delete_module,emqx_cover}]},
    {"4.4.10",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -557,6 +577,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.9",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -584,6 +605,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.8",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -612,6 +634,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.7",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -640,6 +663,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.6",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -668,6 +692,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.5",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_broker,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
@@ -698,6 +723,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.4",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {load_module,emqx_packet,brutal_purge,soft_purge,[]},
@@ -734,6 +760,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.3",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {load_module,emqx_listeners,brutal_purge,soft_purge,[]},
@@ -775,6 +802,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.2",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_banned,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {load_module,emqx_listeners,brutal_purge,soft_purge,[]},
@@ -817,6 +845,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.1",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]},
      {load_module,emqx_router_helper,brutal_purge,soft_purge,[]},
@@ -863,6 +892,7 @@
      {delete_module,emqx_ocsp_cache}]},
    {"4.4.0",
     [{load_module,emqx_vm,brutal_purge,soft_purge,[]},
+     {load_module,emqx_keepalive,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_actions_trans,brutal_purge,soft_purge,[]},
      {load_module,emqx_kernel_sup,brutal_purge,soft_purge,[]},
      {load_module,emqx_router_helper,brutal_purge,soft_purge,[]},

diff --git a/src/emqx_keepalive.erl b/src/emqx_keepalive.erl
@@ -73,6 +73,7 @@ check(NewVal, KeepAlive = #keepalive{statval = OldVal,
         true -> {error, timeout}
     end.
 
+-define(IS_KEEPALIVE(Interval), Interval >= 0 andalso Interval =< 65535000).
 %% from mqtt-v3.1.1 specific
 %% A Keep Alive value of zero (0) has the effect of turning off the keep alive mechanism.
 %% This means that, in this case, the Server is not required
@@ -85,7 +86,10 @@ check(NewVal, KeepAlive = #keepalive{statval = OldVal,
 %% typically this is a few minutes.
 %% The maximum value is (65535s) 18 hours 12 minutes and 15 seconds.
 
-%% @doc Update keepalive's interval
--spec(set(interval, non_neg_integer(), keepalive()) -> keepalive()).
-set(interval, Interval, KeepAlive) when Interval >= 0 andalso Interval =< 65535000 ->
+%% @doc Update keepalive interval
+%% The keepalive() is undefined when connecting via keepalive=0.
+-spec(set(interval, non_neg_integer(), keepalive() | undefined) -> keepalive()).
+set(interval, Interval, undefined) when ?IS_KEEPALIVE(Interval) ->
+    init(Interval);
+set(interval, Interval, KeepAlive) when ?IS_KEEPALIVE(Interval) ->
     KeepAlive#keepalive{interval = Interval}.