Merge pull request #1487 from petermm/fix-network-races

Fix network.erl race conditions

As evidenced by failing simtest CI on release-0.6 etc.

1. startup should use continue/handle_continue and avoid races + cleaner DRY
code.

2. network:stop was using nonblocking call to stop network_port- so rapid
stop/start would give the last network:start an old whereis(network_port) that
was still shutting down, and lead to errors.

3. somewhat unrelated: sta_rssi was using get_port(), and would crash on no
network started - guarded it erlang side, added test.

These changes are made under both the "Apache 2.0" and the "GNU Lesser General
Public License 2.1 or later" license terms (dual license).

SPDX-License-Identifier: Apache-2.0 OR LGPL-2.1-or-later

Author: bettio

CHANGELOG.md

@@ -38,6 +38,8 @@ certain VM instructions are used.
 shallow comparison was performed, so it was working just for plain values such as atoms and small
 integers
 - Fixed support for setting esp32 boot_path in NVS.
+- Fixed race conditions in network:start/stop.
+- Fixed crash calling network:sta_rssi(), when network not up.
 
 ## [0.6.5] - 2024-10-15
 

libs/eavmlib/src/network.erl

@@ -32,6 +32,7 @@
     init/1,
     handle_call/3,
     handle_cast/2,
+    handle_continue/2,
     handle_info/2,
     terminate/2
 ]).
@@ -270,31 +271,11 @@ wait_for_ap(ApConfig, Timeout) ->
 %%-----------------------------------------------------------------------------
 -spec start(Config :: network_config()) -> {ok, pid()} | {error, Reason :: term()}.
 start(Config) ->
-    case gen_server:start({local, ?MODULE}, ?MODULE, Config, []) of
-        {ok, Pid} = R ->
-            case gen_server:call(Pid, start) of
-                ok ->
-                    R;
-                Error ->
-                    Error
-            end;
-        Error ->
-            Error
-    end.
+    gen_server:start({local, ?MODULE}, ?MODULE, Config, []).
 
 -spec start_link(Config :: network_config()) -> {ok, pid()} | {error, Reason :: term()}.
 start_link(Config) ->
-    case gen_server:start_link({local, ?MODULE}, ?MODULE, Config, []) of
-        {ok, Pid} = R ->
-            case gen_server:call(Pid, start) of
-                ok ->
-                    R;
-                Error ->
-                    Error
-            end;
-        Error ->
-            Error
-    end.
+    gen_server:start_link({local, ?MODULE}, ?MODULE, Config, []).
 
 %%-----------------------------------------------------------------------------
 %% @returns ok, if the network interface was stopped, or {error, Reason} if
@@ -314,13 +295,17 @@ stop() ->
 %%-----------------------------------------------------------------------------
 -spec sta_rssi() -> {ok, Rssi :: db()} | {error, Reason :: term()}.
 sta_rssi() ->
-    Port = get_port(),
-    Ref = make_ref(),
-    Port ! {self(), Ref, rssi},
-    receive
-        {Ref, {error, Reason}} -> {error, Reason};
-        {Ref, {rssi, Rssi}} -> {ok, Rssi};
-        Other -> {error, Other}
+    case whereis(network_port) of
+        undefined ->
+            {error, network_down};
+        Port ->
+            Ref = make_ref(),
+            Port ! {self(), Ref, rssi},
+            receive
+                {Ref, {error, Reason}} -> {error, Reason};
+                {Ref, {rssi, Rssi}} -> {ok, Rssi};
+                Other -> {error, Other}
+            end
     end.
 
 %%
@@ -329,28 +314,23 @@ sta_rssi() ->
 
 %% @hidden
 init(Config) ->
-    {ok, #state{config = Config}}.
-
-%% @hidden
-handle_call(start, From, #state{config = Config} = State) ->
     Port = get_port(),
     Ref = make_ref(),
-    Port ! {self(), Ref, {start, Config}},
-    wait_start_reply(Ref, From, Port, State);
-handle_call(_Msg, _From, State) ->
-    {reply, {error, unknown_message}, State}.
+    {ok, #state{config = Config, port = Port, ref = Ref}, {continue, start_port}}.
 
-%% @private
-wait_start_reply(Ref, From, Port, State) ->
+handle_continue(start_port, #state{config = Config, port = Port, ref = Ref} = State) ->
+    Port ! {self(), Ref, {start, Config}},
     receive
         {Ref, ok} ->
-            gen_server:reply(From, ok),
-            {noreply, State#state{port = Port, ref = Ref}};
-        {Ref, {error, Reason} = ER} ->
-            gen_server:reply(From, {error, Reason}),
-            {stop, {start_failed, Reason}, ER, State}
+            {noreply, State};
+        {Ref, {error, Reason}} ->
+            {stop, {start_port_failed, Reason}, State}
     end.
 
+%% @hidden
+handle_call(_Msg, _From, State) ->
+    {reply, {error, unknown_message}, State}.
+
 %% @hidden
 handle_cast(_Msg, State) ->
     {noreply, State}.
@@ -390,10 +370,25 @@ handle_info(Msg, State) ->
     {noreply, State}.
 
 %% @hidden
-terminate(_Reason, _State) ->
+%% Wait for port to be closed
+terminate(_Reason, State) ->
     Ref = make_ref(),
+    Port = State#state.port,
+    PortMonitor = erlang:monitor(port, Port),
     network_port ! {?SERVER, Ref, stop},
-    ok.
+    wait_for_port_close(PortMonitor, Port).
+
+wait_for_port_close(PortMonitor, Port) ->
+    receive
+        {'DOWN', PortMonitor, port, Port, _DownReason} ->
+            ok;
+        _Other ->
+            % Handle unexpected messages if necessary
+            wait_for_port_close(PortMonitor, Port)
+        % Timeout after 1 second just in case.
+    after 1000 ->
+        {error, timeout}
+    end.
 
 %%
 %% Internal operations

src/platforms/esp32/components/avm_builtins/network_driver.c

@@ -79,6 +79,7 @@ static const char *const sta_connected_atom = ATOM_STR("\xD", "sta_connected");
 static const char *const sta_beacon_timeout_atom = ATOM_STR("\x12", "sta_beacon_timeout");
 static const char *const sta_disconnected_atom = ATOM_STR("\x10", "sta_disconnected");
 static const char *const sta_got_ip_atom = ATOM_STR("\xA", "sta_got_ip");
+static const char *const network_down_atom = ATOM_STR("\x0C", "network_down");
 
 ESP_EVENT_DECLARE_BASE(sntp_event_base);
 ESP_EVENT_DEFINE_BASE(sntp_event_base);
@@ -847,13 +848,24 @@ static void get_sta_rssi(Context *ctx, term pid, term ref)
     size_t tuple_reply_size = PORT_REPLY_SIZE + TUPLE_SIZE(2);
 
     int sta_rssi = 0;
-    esp_err_t err = esp_wifi_sta_get_rssi(&sta_rssi);
-    if (UNLIKELY(err != ESP_OK)) {
-        term error_term = term_from_int(err);
-        ESP_LOGE(TAG, "error obtaining RSSI: [%i] %u", err, error_term);
-        // Reply: {Ref, {error, Reason}}
+    wifi_ap_record_t ap_info;
+    esp_err_t status = esp_wifi_sta_get_ap_info(&ap_info);
+    if (status == ESP_OK) {
+        esp_err_t err = esp_wifi_sta_get_rssi(&sta_rssi);
+        if (UNLIKELY(err != ESP_OK)) {
+            term error_term = term_from_int(err);
+            ESP_LOGE(TAG, "error obtaining RSSI: [%i] %u", err, error_term);
+            // Reply: {Ref, {error, Reason}}
+            port_ensure_available(ctx, tuple_reply_size);
+            term error = port_create_error_tuple(ctx, error_term);
+            port_send_reply(ctx, pid, ref, error);
+            return;
+        }
+    } else {
+        ESP_LOGE(TAG, "Device is not connected to any AP.");
+        // Reply: {Ref, {error, network_down}}
         port_ensure_available(ctx, tuple_reply_size);
-        term error = port_create_error_tuple(ctx, error_term);
+        term error = port_create_error_tuple(ctx, make_atom(ctx->global, network_down_atom));
         port_send_reply(ctx, pid, ref, error);
         return;
     }

src/platforms/esp32/test/main/test_erl_sources/test_wifi_example.erl

@@ -25,6 +25,8 @@
 start() ->
     case verify_platform(atomvm:platform()) of
         ok ->
+            % test that sta_rssi() can be safely called, when network is not started
+            {error, network_down} = network:sta_rssi(),
             ok = start_network(),
             ok = network:stop(),
             start_network(),
@@ -59,7 +61,11 @@ start_network() ->
     ],
     case network:start(Config) of
         {ok, _Pid} ->
-            io:format("Network started.~n");
+            % test that sta_rssi() can be safely called
+            % when network is just started, but may not yet be connected.
+            network:sta_rssi(),
+            io:format("Network started.~n"),
+            ok;
         Error ->
             Error
     end.