Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix network.erl race conditions #1487

Open
wants to merge 3 commits into
base: release-0.6
Choose a base branch
from
+66 −53
Open

Fix network.erl race conditions #1487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca9e81b
Network.erl use handle_continue
petermm Jan 22, 2025
749facb
Network.erl fix sta_rssi() crash when not started.
petermm Jan 22, 2025
03dab59
Network.erl fix stop returning too early
petermm Jan 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 41 additions & 46 deletions libs/eavmlib/src/network.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
init/1,
handle_call/3,
handle_cast/2,
handle_continue/2,
handle_info/2,
terminate/2
]).
Expand Down Expand Up @@ -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
Expand All @@ -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(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could also be a monitor on the port.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand, this can be called with no network gen_server nor port started, so not sure where a monitor fits in heh (but I'm certainly no expert) - let's make it work first, then make it beautiful;-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The monitor will tell you if the port disappeared since the call to whereis/1: you can monitor a gone process or port and you will get a DOWN message.

The pattern is the following:

Ref = monitor(port, Port),
Port ! {self(), Ref, ...},
Result = receive
   {Ref, ...} ->
   {'DOWN', Ref, port, Port, Reason} -> ...
end,
demonitor(Ref, [flush]),
Result

Port ! {self(), Ref, rssi},
receive
{Ref, {error, Reason}} -> {error, Reason};
{Ref, {rssi, Rssi}} -> {ok, Rssi};
Other -> {error, Other}
end
end.

%%
Expand All @@ -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}.
Expand Down Expand Up @@ -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
Expand Down
24 changes: 18 additions & 6 deletions src/platforms/esp32/components/avm_builtins/network_driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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;
}
Expand Down
8 changes: 7 additions & 1 deletion src/platforms/esp32/test/main/test_erl_sources/test_wifi_example.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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.
Expand Down
Loading