Skip to content

estdlib: add gen:start/5,6 - #2386

Open
pguyot wants to merge 1 commit into
atomvm:release-0.7from
pguyot:w32/gen-start
Open

estdlib: add gen:start/5,6#2386
pguyot wants to merge 1 commit into
atomvm:release-0.7from
pguyot:w32/gen-start

Conversation

@pguyot

@pguyot pguyot commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

The Elixir GenServer implementation calls them. The monitor linkage folds proc_lib:start_monitor/3's {Result, MonitorRef} into OTP's {ok, {Pid, Mon}}.

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

@petermm

This comment was marked as outdated.

@petermm

petermm commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

preexisting and nitpicks:

PR re-review: abda2ef22 estdlib: add gen:start/5,6

Recommendation: request changes

The contributor fixed the three runtime issues from the first review: named-start failures now
use init_fail, timeout/spawn_opt are forwarded, and init/1 -> ignore is supported.
However, forwarding spawn_opt exposes an existing proc_lib cleanup defect for the valid
link spawn option. A timed-out start can kill its caller or leak an EXIT, so this remains a
merge blocker.

This re-review compared 39aeb940d..abda2ef22 with Erlang/OTP's stdlib/src/gen.erl,
gen_server.erl, and proc_lib.erl in /Users/petermm/OSScontrib/otp, and included a second
independent Oracle review.

Status of previous findings

  1. Named link/monitor failure cleanup: runtime fixed. gen:start/6 now has the OTP-style
    name precheck, and gen_server:init_it/5 uses proc_lib:init_fail/3 after a registration
    race (libs/estdlib/src/gen_server.erl:122-146). The new test covers the precheck but not the
    race path; see the test gaps below.
  2. timeout and spawn_opt: forwarding fixed, cleanup not yet safe. The options reach the
    existing proc_lib:start*/5 APIs (libs/estdlib/src/gen.erl:85-99). Ordinary finite timeout
    behavior is tested and works, but spawn_opt = [link] exposes finding 1.
  3. init/1 -> ignore: runtime fixed. Named, unnamed, and monitored starts now return
    ignore and clean up the child. The public types remain inaccurate; see finding 2.

Findings

1. High — forwarding spawn_opt = [link] exposes unsafe proc_lib cleanup

gen:start(..., nolink, ..., [{spawn_opt, [link]}]) is valid: both AtomVM and OTP include
link in proc_lib:start_spawn_option(). The new code forwards it to proc_lib:start/5 at
libs/estdlib/src/gen.erl:86-94.

AtomVM's proc_lib:start/5, however, calls start0(..., Link = false, Monitor = false) even
when SpawnOpts contains link (libs/estdlib/src/proc_lib.erl:141-143). On timeout, the
Link = false branch kills the child without first unlinking it (proc_lib.erl:238-252): a
non-trapping caller can be killed instead of receiving {error, timeout}, while a trapping
caller can retain a stray EXIT. The same stale boolean skips EXIT cleanup after init_fail.

There is a related pre-ack failure bug in the same state machine. start_monitor/5 passes
Link = true despite not linking by default (proc_lib.erl:189-191). If a child dies before
acknowledgement, the DOWN branch at lines 225-233 consumes one DOWN and then waits forever
for a second one. A trapped start_link failure reaches the same branch.

OTP avoids inferring whether a link exists: every failure path unconditionally calls
unlink(Pid), selectively flushes a matching EXIT, and waits for exactly one DOWN. Fix the
source of truth in proc_lib, rather than filtering link in gen:

diff --git a/libs/estdlib/src/proc_lib.erl b/libs/estdlib/src/proc_lib.erl
@@
 start(Module, Function, Args, Timeout, SpawnOpts) ->
-    start0(Module, Function, Args, Timeout, SpawnOpts, false, false).
+    start0(Module, Function, Args, Timeout, SpawnOpts, false).
@@
 start_link(Module, Function, Args, Timeout, SpawnOpts) ->
-    start0(Module, Function, Args, Timeout, [link | SpawnOpts], true, false).
+    start0(Module, Function, Args, Timeout, [link | SpawnOpts], false).
@@
 start_monitor(Module, Function, Args, Timeout, SpawnOpts) ->
-    start0(Module, Function, Args, Timeout, SpawnOpts, true, true).
+    start0(Module, Function, Args, Timeout, SpawnOpts, true).
 
 %% @private
-start0(Module, Function, Args, Timeout, SpawnOpts, Link, Monitor) ->
+start0(Module, Function, Args, Timeout, SpawnOpts, Monitor) ->
@@
         {nack, Pid, Result} when Monitor ->
+            flush_exit(Pid),
             receive
                 {'DOWN', MonitorRef, process, Pid, _} -> ok
             end,
-            flush_exit(Pid, Link),
             {Result, MonitorRef};
         {nack, Pid, Result} ->
+            flush_exit(Pid),
             receive
                 {'DOWN', MonitorRef, process, Pid, _} -> ok
             end,
-            flush_exit(Pid, Link),
             Result;
-        {'DOWN', MonitorRef, process, Pid, Reason} when Link ->
-            receive
-                {'EXIT', Pid, _} -> ok
-            after 0 -> ok
-            end,
-            receive
-                {'DOWN', MonitorRef, process, Pid, _} -> ok
-            end,
-            {error, Reason};
         {'DOWN', MonitorRef, process, Pid, Reason} when Monitor ->
+            flush_exit(Pid),
             {{error, Reason}, MonitorRef};
         {'DOWN', MonitorRef, process, Pid, Reason} ->
+            flush_exit(Pid),
             {error, Reason}
     after Timeout ->
-        if
-            Link ->
-                unlink(Pid),
-                exit(Pid, kill),
-                receive
-                    {'EXIT', Pid, _} -> ok
-                after 0 -> ok
-                end;
-            true ->
-                exit(Pid, kill)
-        end,
+        unlink(Pid),
+        exit(Pid, kill),
+        flush_exit_message(Pid),
         receive
             {'DOWN', MonitorRef, process, Pid, _} -> ok
         end,
@@
-flush_exit(_Pid, false) ->
-    ok;
-flush_exit(Pid, true) ->
+flush_exit(Pid) ->
+    unlink(Pid),
+    flush_exit_message(Pid).
+
+flush_exit_message(Pid) ->
     receive
         {'EXIT', Pid, _} -> ok
     after 0 -> ok
     end.

Add timeout/failure tests for plain link, plain monitor, and nolink with
{spawn_opt, [link]}. Run each start in an isolated trapping process, assert the return shape,
assert the child is dead, and selectively assert that no unexpected lifecycle message remains.

2. Medium — ignore is missing from the callback and public return types

The implementation now returns ignore, including through the public gen_server:start* APIs,
but start_ret/0, start_mon_ret/0, and the init/1 callback result exclude it
(libs/estdlib/src/gen_server.erl:70-79). This gives callers and callback modules an incorrect
Dialyzer contract.

diff --git a/libs/estdlib/src/gen_server.erl b/libs/estdlib/src/gen_server.erl
@@
-type start_ret() :: {ok, pid()} | {error, Reason :: term()}.
-type start_mon_ret() :: {ok, {Pid :: pid(), MonRef :: reference()}} | {error, Reason :: term()}.
+-type start_ret() :: {ok, pid()} | ignore | {error, Reason :: term()}.
+-type start_mon_ret() ::
+    {ok, {Pid :: pid(), MonRef :: reference()}} | ignore | {error, Reason :: term()}.
@@
 -type init_result(StateType) ::
     {ok, State :: StateType}
     | {ok, State :: StateType, timeout() | {timeout, timeout(), Msg :: any()} | {continue, term()}}
-    | {stop, Reason :: any()}.
+    | {stop, Reason :: any()}
+    | ignore.

3. Medium — tuple-form monitor spawn options are not rejected

proc_lib:start*/5 documents monitor spawn options as forbidden, but the validation at
libs/estdlib/src/proc_lib.erl:194-198 rejects only the atom monitor. AtomVM's spawn options
also support {monitor, MonitorOpts}; prepending the internal monitor to that malformed list can
produce the wrong return shape or silently lose monitor-option semantics. OTP rejects both
forms.

diff --git a/libs/estdlib/src/proc_lib.erl b/libs/estdlib/src/proc_lib.erl
@@
-    case lists:member(monitor, SpawnOpts) of
+    case lists:member(monitor, SpawnOpts) orelse lists:keymember(monitor, 1, SpawnOpts) of
         true -> error(badarg);
         false -> ok
     end,

Test gaps

  • test_gen_start_named_occupied/0 sets up an already-occupied name, so gen:start/6 returns
    from its whereis/1 fast path. It does not execute gen_server:init_it/5 or prove the
    registration-race init_fail cleanup. A barrier-controlled concurrent registration test is
    needed for that branch.
  • test_gen_start_ignore/0 covers nolink and monitor, but not link.
  • test_gen_start_timeout/0 covers only plain nolink; no test exercises a spawn option.
  • drain_mailbox/0 consumes every message from the shared test process and _ = drain_mailbox() explicitly discards evidence. Prefer isolated starter processes and selective
    matching by known child PID/reference; this avoids hiding unrelated protocol messages and the
    timing assumptions of a 100 ms quiet period.

Verification

  • git diff --check 39aeb940d..HEAD and git diff --check HEAD^ HEAD — passed.
  • cmake --build build --target test_estdlib -j4 — passed (one existing deprecated-catch
    warning).
  • ./build/src/AtomVM ./build/tests/libs/estdlib/test_estdlib.avm — passed; every listed module,
    including test_gen_server and test_proc_lib, returned ok. The green suite does not cover
    the spawn_opt = [link], pre-ack failure, or registration-race paths described above.

The Elixir GenServer implementation calls them. The monitor linkage folds
proc_lib:start_monitor/3's {Result, MonitorRef} into OTP's {ok, {Pid, Mon}}.

A named start no longer spawns a child when the name is taken, and a failed
registration terminates the child through init_fail so no EXIT or DOWN is left
in the caller's mailbox. init/1 returning ignore now yields ignore, and the
timeout and spawn_opt options are honoured.

Forwarding spawn_opt made proc_lib's failure cleanup reachable with a link the
caller did not ask for, so it now unlinks unconditionally like OTP instead of
tracking whether it created the link.

Signed-off-by: Paul Guyot <pguyot@kallisys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants