From 89918dc165c17e08ef9fabc84095effb96ebc60b Mon Sep 17 00:00:00 2001 From: Made In Heaven Date: Thu, 16 Jul 2026 13:31:34 +1000 Subject: [PATCH] Fix: timer_manager:send_after/3 returns an uncancellable timer send_after/3 minted a fresh reference with erlang:make_ref/0 and returned it, but registered the actual timer under a different reference created by an intermediate proxy process. As a result the returned reference was never present in the timer manager's table, so cancel_timer/1 always returned false and the scheduled message was delivered even after a cancel. The proxy process also had no cancel path, making send_after timers structurally uncancellable. This affects erlang:send_after/3 and timer:send_after/2,3, which delegate to it. Route send_after/3 through the same gen_server timer path as start_timer/3, tagging the timer with a delivery mode so it delivers the bare message instead of a {timeout, Ref, Msg} tuple. The returned reference is now registered and cancellable, and cancel_timer/1 returns the remaining milliseconds and suppresses delivery. Add regression tests covering cancellation of a send_after timer and that a cancelled send_after timer does not deliver its message. 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 Signed-off-by: Made In Heaven --- CHANGELOG.md | 3 ++ libs/eavmlib/src/timer_manager.erl | 43 ++++++++++++----------- tests/libs/eavmlib/test_timer_manager.erl | 29 +++++++++++++++ 3 files changed, 54 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 29b81ae4e4..6fccdc3c40 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -64,6 +64,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fixed several underallocation issues that could trigger data corruption on `binary:replace`, `zlib:compress` and bsd socket recv code. - Fixed a bug where `catch` would raise on regular atom results - Fixed ESP32 socket driver holding the global socket-list lock across blocking TCP connects, leaking the port on connect failure, losing concurrent `accept` waiters, leaking `netbuf` on receive error paths, and a recycled-`netconn` race between socket close and the event handler +- Fixed `timer_manager:send_after/3` (and therefore `erlang:send_after/3` and `timer:send_after/2,3`) + returning a reference that was never registered with the timer manager, so `cancel_timer/1` always + returned `false` and the scheduled message was delivered regardless of cancellation ## [0.7.0-alpha.1] - 2026-04-06 diff --git a/libs/eavmlib/src/timer_manager.erl b/libs/eavmlib/src/timer_manager.erl index 6154c94c52..aa239e87ac 100644 --- a/libs/eavmlib/src/timer_manager.erl +++ b/libs/eavmlib/src/timer_manager.erl @@ -25,7 +25,7 @@ -export([start/0, start_timer/3, cancel_timer/1, get_timer_refs/0, send_after/3]). -export([init/1, handle_call/3, handle_cast/2, handle_info/2, terminate/2]). --export([run_timer/5, send_after_timer/3]). +-export([run_timer/6]). -record(state, { timers = [] :: [{reference(), pid()}] @@ -57,10 +57,11 @@ maybe_start() -> TimerRef :: reference(). start_timer(Time, Dest, Msg) -> maybe_start(), - gen_server:call(?SERVER_NAME, {Time, Dest, Msg}). + gen_server:call(?SERVER_NAME, {start_timer, Time, Dest, Msg}). %%----------------------------------------------------------------------------- -%% @param TimerRef a timer reference returned from {@link start_timer/3}. +%% @param TimerRef a timer reference returned from {@link start_timer/3} or +%% {@link send_after/3}. %% @returns the time in milliseconds left until the timer would have expired, %% or `false' if the timer was not found. %% @doc Cancel a timer. @@ -81,14 +82,16 @@ get_timer_refs() -> %% @param Dest Pid or server name to which to send the message. %% @param Msg Message to send to Dest after Time ms. %% @returns a reference that can be used to cancel the timer, if desired. -%% @doc Send Msg to Dest after Time ms. +%% @doc Send Msg to Dest after Time ms. Unlike {@link start_timer/3}, the +%% bare Msg is delivered (not wrapped in a `{timeout, Ref, Msg}' +%% tuple). The returned reference is registered with the timer +%% manager and can be cancelled with {@link cancel_timer/1}. %% @end %%----------------------------------------------------------------------------- -spec send_after(non_neg_integer(), pid() | atom(), term()) -> reference(). send_after(Time, Dest, Msg) -> - TimerRef = erlang:make_ref(), - spawn(?MODULE, send_after_timer, [Time, Dest, Msg]), - TimerRef. + maybe_start(), + gen_server:call(?SERVER_NAME, {send_after, Time, Dest, Msg}). %% %% ?GEN_SERVER callbacks @@ -111,8 +114,11 @@ handle_call({cancel, TimerRef}, From, #state{timers = Timers} = State) -> NewTimers = lists:keyreplace(Pid, 2, Timers, {{canceled, From}, Pid}), {noreply, State#state{timers = NewTimers}} end; -handle_call({Time, Dest, Msg}, _From, #state{timers = Timers} = State) -> - {TimerRef, Pid} = do_start_timer(Time, Dest, Msg), +handle_call({start_timer, Time, Dest, Msg}, _From, #state{timers = Timers} = State) -> + {TimerRef, Pid} = do_start_timer(Time, Dest, Msg, wrapped), + {reply, TimerRef, State#state{timers = [{TimerRef, Pid} | Timers]}}; +handle_call({send_after, Time, Dest, Msg}, _From, #state{timers = Timers} = State) -> + {TimerRef, Pid} = do_start_timer(Time, Dest, Msg, bare), {reply, TimerRef, State#state{timers = [{TimerRef, Pid} | Timers]}}. %% @hidden @@ -153,13 +159,13 @@ terminate(_Reason, _State) -> %% internal functions %% @private -do_start_timer(Time, Dest, Msg) -> +do_start_timer(Time, Dest, Msg, Mode) -> TimerRef = erlang:make_ref(), - Pid = spawn(?MODULE, run_timer, [self(), Time, TimerRef, Dest, Msg]), + Pid = spawn(?MODULE, run_timer, [self(), Time, TimerRef, Dest, Msg, Mode]), {TimerRef, Pid}. %% @private -run_timer(MgrPid, Time, TimerRef, Dest, Msg) -> +run_timer(MgrPid, Time, TimerRef, Dest, Msg, Mode) -> Start = erlang:system_time(millisecond), receive {cancel, From} -> @@ -167,13 +173,8 @@ run_timer(MgrPid, Time, TimerRef, Dest, Msg) -> gen_server:reply(From, Time - (erlang:system_time(millisecond) - Start)) after Time -> MgrPid ! {fired, self()}, - Dest ! {timeout, TimerRef, Msg} - end. - -%% @private -send_after_timer(Time, Dest, Msg) -> - TimerRef = start_timer(Time, self(), Msg), - receive - {timeout, TimerRef, Msg} -> - Dest ! Msg + case Mode of + wrapped -> Dest ! {timeout, TimerRef, Msg}; + bare -> Dest ! Msg + end end. diff --git a/tests/libs/eavmlib/test_timer_manager.erl b/tests/libs/eavmlib/test_timer_manager.erl index edb9501fcc..1af6b0a784 100644 --- a/tests/libs/eavmlib/test_timer_manager.erl +++ b/tests/libs/eavmlib/test_timer_manager.erl @@ -28,6 +28,8 @@ test() -> ok = test_cancel_timer_after_expiry(), ok = test_erlang_cancel_timer(), pong = test_send_after(), + ok = test_cancel_send_after(), + ok = test_cancel_send_after_suppresses_message(), ok. -include("etest.hrl"). @@ -73,6 +75,33 @@ test_send_after() -> timer_manager:send_after(100, self(), ping), pong = wait_for_timeout(ping, 5000). +%% Regression: the reference returned by send_after/3 must be a registered, +%% cancellable timer. It previously returned a throwaway ref that was never +%% in the timer table, so cancel_timer/1 always returned false. +test_cancel_send_after() -> + ?ASSERT_MATCH(timer_manager:get_timer_refs(), []), + TimerRef = timer_manager:send_after(60000, self(), test_cancel_send_after), + ?ASSERT_MATCH(timer_manager:get_timer_refs(), [TimerRef]), + R = timer_manager:cancel_timer(TimerRef), + ?ASSERT_TRUE(is_integer(R)), + ?ASSERT_TRUE(R > 0), + ?ASSERT_MATCH(timer_manager:get_timer_refs(), []), + R2 = timer_manager:cancel_timer(TimerRef), + ?ASSERT_EQUALS(false, R2), + ok. + +%% Regression: cancelling a send_after/3 timer must actually stop the message +%% from being delivered. It previously fired regardless of the cancel. +test_cancel_send_after_suppresses_message() -> + ?ASSERT_MATCH(timer_manager:get_timer_refs(), []), + TimerRef = timer_manager:send_after(100, self(), should_not_arrive), + _ = timer_manager:cancel_timer(TimerRef), + receive + should_not_arrive -> throw(timer_fired_despite_cancel) + after 500 -> ok + end, + ok. + wait_for_timeout(Msg, Timeout) -> receive {timeout, _TimerRef, Msg} ->