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

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

This comment was marked as outdated.

@pguyot
pguyot force-pushed the w32/gen-start branch 2 times, most recently from 1efb84e to cd8cac1 Compare August 5, 2026 05:31
@petermm

petermm commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

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

Recommendation: request changes

The contributor fixed the previous blocker: timeout and failure cleanup now handles links from
both start_link and spawn_opt, no path waits for a second DOWN, ignore types are correct,
and both monitor spawn-option forms are rejected. One monitor-ownership mismatch remains:
failed proc_lib:start_monitor/* calls consume the terminal DOWN before returning its monitor
reference, contrary to the documented OTP contract. This should be fixed before merge.

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

Status of previous findings

  • Stale link tracking and unsafe timeout cleanup: fixed. Failure paths now unlink
    unconditionally, selectively flush EXIT, and await exactly one DOWN. A caller-supplied
    {spawn_opt, [link]} is handled the same way as start_link.
  • Pre-ack start_link/start_monitor hang: fixed. The branch no longer consumes one DOWN
    and waits for another.
  • Missing ignore callback and return types: fixed. start_ret/0, start_mon_ret/0, and
    init_result/1 now include ignore (libs/estdlib/src/gen_server.erl:70-81).
  • Tuple monitor option validation: fixed. Both monitor and {monitor, _} are rejected
    before spawning (libs/estdlib/src/proc_lib.erl:194-198), with a regression test.

Finding

High — failed proc_lib:start_monitor/* calls lose the returned monitor's DOWN

The Monitor = true failure branches return {Result, MonitorRef} after consuming the only
terminal DOWN:

  • init_fail/nack: libs/estdlib/src/proc_lib.erl:213-218
  • pre-ack child death: libs/estdlib/src/proc_lib.erl:225-227
  • timeout: libs/estdlib/src/proc_lib.erl:231-243

This violates the direct proc_lib:start_monitor/* contract. OTP explicitly documents that if
the function returns an error, a DOWN is still delivered to the caller, including after a
timeout. Its sync_start_monitor/2 re-enqueues the exact consumed DOWN before returning the
reference (/Users/petermm/OSScontrib/otp/lib/stdlib/src/proc_lib.erl:582-598). A direct AtomVM
caller that waits on the returned monitor after a failed start will instead block forever.

The higher-level APIs have the opposite ownership rule: gen:start(..., monitor, ...) and
gen_server:start_monitor(...) hide the reference on failure, so they must consume the
preserved DOWN before returning. OTP does this in gen:monitor_return/1. AtomVM currently gets
a clean mailbox only because proc_lib swallows the message too early.

Preserve DOWN at the proc_lib boundary:

diff --git a/libs/estdlib/src/proc_lib.erl b/libs/estdlib/src/proc_lib.erl
@@
         {nack, Pid, Result} when Monitor ->
             flush_exit(Pid),
-            receive
-                {'DOWN', MonitorRef, process, Pid, _} -> ok
-            end,
+            Down = receive
+                {'DOWN', MonitorRef, process, Pid, _} = D -> D
+            end,
+            self() ! Down,
             {Result, MonitorRef};
@@
-        {'DOWN', MonitorRef, process, Pid, Reason} when Monitor ->
+        {'DOWN', MonitorRef, process, Pid, Reason} = Down when Monitor ->
             flush_exit(Pid),
+            self() ! Down,
             {{error, Reason}, MonitorRef};
@@
-        receive
-            {'DOWN', MonitorRef, process, Pid, _} -> ok
-        end,
+        Down = receive
+            {'DOWN', MonitorRef, process, Pid, _} = D -> D
+        end,
         case Monitor of
             true ->
+                self() ! Down,
                 {{error, timeout}, MonitorRef};

Then consume that DOWN only where the monitor reference is hidden. For gen:

diff --git a/libs/estdlib/src/gen.erl b/libs/estdlib/src/gen.erl
@@
     case proc_lib:start_monitor(GenMod, init_it, InitArgs, Timeout, SpawnOpts) of
         {{ok, Pid}, Mon} -> {ok, {Pid, Mon}};
-        {Error, _Mon} -> Error
+        {Error, Mon} ->
+            receive
+                {'DOWN', Mon, process, _Pid, _Reason} -> Error
+            end
     end.

Apply the same failure receive to both public gen_server:start_monitor overloads. A small local
helper can avoid duplicating the success/error folding:

diff --git a/libs/estdlib/src/gen_server.erl b/libs/estdlib/src/gen_server.erl
@@
 start_monitor(Module, Args, Options) ->
-    {Result, Monitor} = proc_lib:start_monitor(?MODULE, init_it, [self(), Module, Args, Options]),
-    case Result of
-        {ok, Pid} ->
-            {ok, {Pid, Monitor}};
-        _ ->
-            Result
-    end.
+    monitor_start_result(
+        proc_lib:start_monitor(?MODULE, init_it, [self(), Module, Args, Options])
+    ).
+
+monitor_start_result({{ok, Pid}, Monitor}) ->
+    {ok, {Pid, Monitor}};
+monitor_start_result({Result, Monitor}) ->
+    receive
+        {'DOWN', Monitor, process, _Pid, _Reason} -> Result
+    end.

Use monitor_start_result/1 in the named overload after its whereis/1 precheck as well.

Remaining non-blocking compatibility note

If register/2 loses a race and the winner exits before the subsequent whereis/1,
gen_server:init_it/5 returns {error, badarg}. OTP produces
{error, {already_started, undefined}}. This rare error-shape difference has no lifecycle or
resource leak and may reasonably remain an AtomVM subset difference.

Test gaps

  • Add direct failed-proc_lib:start_monitor tests for init_fail, pre-ack crash, and timeout.
    Each must assert that the returned reference receives its terminal DOWN.
  • Add corresponding gen:start(..., monitor, ...) and public gen_server:start_monitor failure
    tests asserting that those higher-level APIs leave no hidden-monitor DOWN behind.
  • test_proc_lib:test_start_crash/0 still covers only unlinked proc_lib:start; add immediate
    crash cases for trapped start_link, start_monitor, and start_monitor(..., [link]) to
    directly guard the former second-DOWN hang.
  • The occupied-name test still covers only the initial whereis/1 fast path, not a registration
    race between the precheck and child register/2.
  • isolated_start/2 demonitor-flushes as soon as it receives {done, ...}. A delayed fatal exit
    after that send could be concealed. Keep the monitor and require a subsequent normal worker
    DOWN; on the eight-second timeout, kill and reap the worker instead of leaving it alive.
  • The shared drain_mailbox/0 remains broad and timing-based. Prefer isolated workers and
    selective matching by known PID/reference for lifecycle assertions.

Verification

  • git show --check 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 direct failed-monitor
    contract and pre-ack crash paths described above remain uncovered.

@pguyot
pguyot force-pushed the w32/gen-start branch 2 times, most recently from cd8cac1 to 4e6c1f2 Compare September 25, 2026 15:10

@bettio bettio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Few changes required.

Also, libs/exavmlib/lib/GenServer.ex:1045 still carries the ":gen.start is not available" workaround and calls :gen_server.start_link/3,4 and :gen_server.start/3,4 directly, so timeout: and spawn_opt: stay ignored for the bundled Elixir GenServer even though this PR now honours them. Since this PR introduces the entry point, it is the right place to use it: make do_start/4 call :gen.start(:gen_server, link, module, init_arg, opts) and :gen.start(:gen_server, link, {:local, atom}, module, init_arg, opts) as upstream Elixir does, and drop the two case link do blocks and the workaround comment. {:global, _} and {:via, _, _} names stay unsupported, as gen:start/6 only accepts {local, Name}.

{'DOWN', MonitorRef, process, Pid, _} -> ok
end,
flush_exit(Pid, Link),
flush_exit(Pid),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

flush_exit/1 now runs before await_down/2 in both nack clauses (lines 219 and 223), so a linked caller is unlinked while the child may still be alive. Whether a non-trapping start_link caller is then killed depends on whether the child's exit signal is already queued when the caller handles the nack.

OTP's gen_server never enters that path: it only nacks with {exit, normal} and exits directly on {stop, Reason}, bad returns and crashes, so the link fires deterministically.

Our gen_server:init_it/4 nacks with {exit, Reason} for all three.

A regression test fits the existing helper: {killed, boom} = isolated(false, fun() -> gen:start(gen_server, link, ?MODULE, {stop, boom}, []) end) plus an init({stop, Reason}) -> {stop, Reason} clause.

Comment thread CHANGELOG.md Outdated
by adding the `elixir_erl_pass` runtime helpers that compiled Elixir code calls
- Fixed `maps:from_keys/2` (and `sets:from_list/1`, which is built on top of it) not
deduplicating structurally equal but separately-allocated boxed terms, such as tuples
- Fixed `gen_server` returning an error instead of `ignore` when `init/1` returns `ignore`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's move this block to beta.1

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.

A failed proc_lib:start_monitor now delivers the DOWN for the monitor it returns,
as OTP does. gen:start and gen_server:start_monitor, which hide that monitor on
failure, consume it through gen:monitor_return/1.

Like OTP, a server whose init/1 stops, crashes or returns a bad value now exits
with that reason instead of nacking, so a linked caller deterministically gets the
exit signal and the start returns {error, Reason}. A value thrown by init/1 is
treated as its return value. Only ignore still goes through init_fail.

The bundled Elixir GenServer now starts through gen:start/5,6, as upstream Elixir
does, so its :timeout and :spawn_opt options are honoured.

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.

3 participants