More fixes (#307)

* Upgrade grpcbox / chatterbox to handle connection exit * Lower logs * Do not make supervisor trigger when gwmp worker goes down for silly things * Fix typo
helium · Oct 9, 2024 · b407be7 · b407be7
1 parent 1a39ce4
commit b407be7
Show file tree

Hide file tree

Showing 8 changed files with 37 additions and 33 deletions.
diff --git a/rebar.lock b/rebar.lock
@@ -17,7 +17,7 @@
  {<<"cf">>,{pkg,<<"cf">>,<<"0.3.1">>},2},
  {<<"chatterbox">>,
   {git,"https://github.com/novalabsxyz/chatterbox",
-       {ref,"3dd6ac8cbfd0034f6e7ffc7e27919351135c6d98"}},
+       {ref,"1576025ed66ad65489f62cd5d897433f192eebf5"}},
   1},
  {<<"clique">>,
   {git,"https://github.com/helium/clique.git",
@@ -54,7 +54,7 @@
  {<<"gproc">>,{pkg,<<"gproc">>,<<"0.9.0">>},0},
  {<<"grpcbox">>,
   {git,"https://github.com/novalabsxyz/grpcbox.git",
-       {ref,"6243151a7c54c714a018b3d0b92dbf057c033730"}},
+       {ref,"955ec0213970acc822d882cf42e9e1f902264bbe"}},
   0},
  {<<"h3">>,
   {git,"https://github.com/helium/erlang-h3.git",

diff --git a/src/protocols/gwmp/hpr_gwmp_sup.erl b/src/protocols/gwmp/hpr_gwmp_sup.erl
@@ -27,8 +27,8 @@
 
 -define(FLAGS, #{
     strategy => simple_one_for_one,
-    intensity => 3,
-    period => 60
+    intensity => 100,
+    period => 1
 }).
 
 %%====================================================================
@@ -38,7 +38,7 @@
 start_link() ->
     supervisor:start_link({local, ?MODULE}, ?MODULE, []).
 
--spec maybe_start_worker(Args :: map()) -> {ok, pid()} | {error, any()}.
+-spec maybe_start_worker(Args :: map()) -> {ok, pid() | undefined} | {error, any()}.
 maybe_start_worker(#{key := Key} = Args) ->
     case gproc:lookup_local_name(Key) of
         Pid when is_pid(Pid) ->

diff --git a/src/protocols/gwmp/hpr_gwmp_worker.erl b/src/protocols/gwmp/hpr_gwmp_worker.erl
@@ -87,18 +87,18 @@ push_data(WorkerPid, PacketUp, SocketDest, Timestamp, GatewayLocation) ->
 %% ------------------------------------------------------------------
 
 init(#{key := Key, pubkeybin := PubKeyBin} = Args) ->
-    lager:info("~p init with ~p", [?SERVER, Args]),
+    lager:debug("~p init with ~p", [?SERVER, Args]),
+
+    lager:md([
+        {packet_gateway, hpr_utils:gateway_name(PubKeyBin)},
+        {gateway_mac, hpr_utils:gateway_mac(PubKeyBin)},
+        {pubkey, libp2p_crypto:bin_to_b58(PubKeyBin)}
+    ]),
 
     try gproc:add_local_name(Key) of
         true ->
             PullDataTimer = maps:get(pull_data_timer, Args, ?PULL_DATA_TIMER),
 
-            lager:md([
-                {packet_gateway, hpr_utils:gateway_name(PubKeyBin)},
-                {gateway_mac, hpr_utils:gateway_mac(PubKeyBin)},
-                {pubkey, libp2p_crypto:bin_to_b58(PubKeyBin)}
-            ]),
-
             {ok, Socket} = gen_udp:open(0, [binary, {active, true}]),
 
             _ = erlang:send_after(500, self(), {monitor_stream, 0}),
@@ -114,7 +114,8 @@ init(#{key := Key, pubkeybin := PubKeyBin} = Args) ->
     catch
         % This will only catch a bad registration
         error:badarg ->
-            {stop, already_registered}
+            lager:warning("already registered", []),
+            ignore
     end.
 
 -spec handle_call(Msg, _From, #state{}) -> {stop, {unimplemented_call, Msg}, #state{}}.
@@ -144,14 +145,14 @@ handle_info({monitor_stream, AttemptCnt}, #state{pubkeybin = PubKeyBin} = State)
     MD = [{attempt, AttemptCnt}],
     case {AttemptCnt, hpr_packet_router_service:locate(PubKeyBin)} of
         {_, {ok, Pid}} ->
-            lager:info(MD, "monitor stream success"),
+            lager:debug(MD, "monitor stream success"),
             _ = erlang:monitor(process, Pid, [{tag, {'DOWN', PubKeyBin}}]),
             {noreply, State};
         {?MONITOR_STREAM_MAX_ATTEMPT, _} ->
-            lager:info(MD, "monitor stream failed at max attempt"),
-            {stop, max_monitor_attempt, State};
+            lager:warning(MD, "monitor stream failed at max attempt"),
+            {stop, normal, State};
         {_, {error, not_found}} ->
-            lager:info(MD, "monitor stream failed"),
+            lager:debug(MD, "monitor stream failed"),
             erlang:send_after(?MONITOR_STREAM_TIMEOUT, self(), {monitor_stream, AttemptCnt + 1}),
             {noreply, State}
     end;
@@ -163,7 +164,7 @@ handle_info(
         {noreply, _} = NoReply -> NoReply
     catch
         _E:_R ->
-            lager:error("failed to handle UDP packet ~p/~p", [_E, _R]),
+            lager:warning("failed to handle UDP packet ~p/~p", [_E, _R]),
             {noreply, State}
     end;
 handle_info(
@@ -211,6 +212,7 @@ handle_info(
     lager:debug("gateway stream ~p went down: ~p waiting ~wms", [_Pid, _ExitReason, ?CLEANUP_TIME]),
     _ = erlang:send_after(?CLEANUP_TIME, self(), ?CLEANUP),
     {noreply, State};
+% TODO: Maybe shutdown right away?
 handle_info(?CLEANUP, #state{pubkeybin = PubKeyBin} = State) ->
     case hpr_packet_router_service:locate(PubKeyBin) of
         {error, _Reason} ->

diff --git a/src/protocols/hpr_protocol_gwmp.erl b/src/protocols/hpr_protocol_gwmp.erl
@@ -24,13 +24,13 @@ send(PacketUp, Route, Timestamp, GatewayLocation, Retry) ->
     Gateway = hpr_packet_up:gateway(PacketUp),
     Key = {?MODULE, Gateway},
     case hpr_gwmp_sup:maybe_start_worker(#{key => Key, pubkeybin => Gateway}) of
+        {error, Reason} ->
+            {error, {gwmp_sup_err, Reason}};
         % This should only happen when a hotspot connects and spams us with
-        % mutliple packet for same LNS
-        {error, already_registered} ->
+        % mutliple packets for same LNS
+        {ok, undefined} ->
             timer:sleep(2),
             send(PacketUp, Route, Timestamp, GatewayLocation, Retry - 1);
-        {error, Reason} ->
-            {error, {gwmp_sup_err, Reason}};
         {ok, Pid} ->
             Region = hpr_packet_up:region(PacketUp),
             Dest = hpr_route:gwmp_region_lns(Region, Route),

diff --git a/src/protocols/hpr_protocol_http_roaming.erl b/src/protocols/hpr_protocol_http_roaming.erl
@@ -34,17 +34,18 @@ send(_PacketUp, _Route, _Timestamp, _GatewayLocation, 0) ->
 send(PacketUp, Route, Timestamp, GatewayLocation, Retry) ->
     Protocol = protocol_from(Route),
     WorkerKey = worker_key_from(PacketUp, Protocol),
-    %% start worker
     case
         hpr_http_roaming_sup:maybe_start_worker(#{
             key => WorkerKey, protocol => Protocol, net_id => hpr_route:net_id(Route)
         })
     of
-        {error, already_registered} ->
-            timer:sleep(2),
-            send(PacketUp, Route, Timestamp, GatewayLocation, Retry - 1);
         {error, Reason} ->
             {error, {roaming_sup_err, Reason}};
+        % This should only happen when a hotspot connects and spams us with
+        % mutliple packets for same LNS
+        {ok, undefined} ->
+            timer:sleep(2),
+            send(PacketUp, Route, Timestamp, GatewayLocation, Retry - 1);
         {ok, WorkerPid} ->
             hpr_http_roaming_worker:handle_packet(WorkerPid, PacketUp, Timestamp, GatewayLocation),
             ok

diff --git a/src/protocols/http/hpr_http_roaming.erl b/src/protocols/http/hpr_http_roaming.erl
@@ -256,7 +256,7 @@ handle_prstart_ans(#{
 }) ->
     NetID = hpr_http_roaming_utils:hexstring_to_int(SenderID),
 
-    lager:info("stop buying [net_id: ~p] [reason: no roaming agreement]", [NetID]),
+    lager:debug("stop buying [net_id: ~p] [reason: no roaming agreement]", [NetID]),
 
     ok;
 handle_prstart_ans(#{
@@ -265,13 +265,13 @@ handle_prstart_ans(#{
     <<"SenderID">> := SenderID
 }) ->
     %% Catchall for properly formatted messages with results we don't yet support
-    lager:info(
+    lager:debug(
         "[result: ~p] [sender: ~p] [description: ~p]",
         [ResultCode, SenderID, maps:get(<<"Description">>, Result, "No Description")]
     ),
     ok;
 handle_prstart_ans(Res) ->
-    lager:error("unrecognized prstart_ans: ~p", [Res]),
+    lager:warning("unrecognized prstart_ans: ~p", [Res]),
     throw({bad_response, Res}).
 
 -spec handle_xmitdata_req(xmitdata_req()) ->

diff --git a/src/protocols/http/hpr_http_roaming_sup.erl b/src/protocols/http/hpr_http_roaming_sup.erl
@@ -35,8 +35,8 @@
 
 -define(FLAGS, #{
     strategy => simple_one_for_one,
-    intensity => 3,
-    period => 60
+    intensity => 100,
+    period => 1
 }).
 
 -export_type([http_protocol/0]).
@@ -50,7 +50,7 @@ start_link() ->
 
 -spec maybe_start_worker(
     Args :: map()
-) -> {ok, pid()} | {error, any()}.
+) -> {ok, pid() | undefined} | {error, any()}.
 maybe_start_worker(#{key := Key} = Args) ->
     case gproc:lookup_local_name(Key) of
         Pid when is_pid(Pid) ->

diff --git a/src/protocols/http/hpr_http_roaming_worker.erl b/src/protocols/http/hpr_http_roaming_worker.erl
@@ -102,7 +102,8 @@ init(Args) ->
     catch
         % This will only catch a bad registration
         error:badarg ->
-            {stop, already_registered}
+            lager:warning("already registered", []),
+            ignore
     end.
 
 handle_call(_Msg, _From, State) ->