[Updated] Add support for process aliases#2326
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
|
||
| term context_process_alias_message_signal(Context *ctx, struct TermSignal *signal) | ||
| { | ||
| // signal->signal_term is a 2-tuple {Ref, Message}. The alias lookup runs here, in the owner's own |
There was a problem hiding this comment.
Do we want comments that verbose? The drawback is that eventually they end up being unsync with code, and LLM or humans tend to read comments instead of code.
There was a problem hiding this comment.
Not at all, I'm leaving them for helping with review activity, but I will cleanup code from all that narration.
| { | ||
| Context *p = globalcontext_get_process_lock(glb, process_id); | ||
| if (p) { | ||
| // Post the alias message as a signal carrying {Ref, Message}. The owner validates the alias |
There was a problem hiding this comment.
I don't think this comment adds any value
There was a problem hiding this comment.
All these comments/narration will be removed while squashing, after review.
| * | ||
| * @details Posts an AliasMessageSignal carrying {Ref, Message} to the owner process. The owner | ||
| * validates the alias against its own monitors when it drains signals and either delivers Message | ||
| * as a normal message or drops it. This avoids touching the owner's monitor list from the sender. |
There was a problem hiding this comment.
Last sentence sounds obvious here.
|
AMP: PR Review — Process aliases for AtomVMRange reviewed: Diffstat: ~40 files, +1975 / −173. The series adds OTP process aliases: VerdictStrong, ship-quality work. The design is sound: aliases are validated in The findings below are mostly pre-existing OOM-robustness gaps that this Findings1. (Medium) OOM in
|
| // Reverse the LIFO list into received order (oldest first), splitting it into a normal sublist | ||
| // and a signal sublist in one pass. This entry is alias-blind: its callers (ports, teardown and | ||
| // crashdump) never own an active alias. | ||
| MailboxMessage *normal_first = NULL; |
There was a problem hiding this comment.
I'm ok with the renaming, but keeping the old names would reduce the diff.
| TERM_DEBUG_ASSERT(ctx->active_alias_count > 0); | ||
| if (LIKELY(ctx->active_alias_count != ACTIVE_ALIAS_COUNT_SATURATED)) { | ||
| ctx->active_alias_count--; | ||
| } |
There was a problem hiding this comment.
I would put this after the list_remove and if the count is saturated but there is no monitor left (after the remove) I would reset it to 0, so the alias count gets a chance to be reset.
| CONTEXT_MONITOR_LINK_REMOTE, | ||
| CONTEXT_MONITOR_MONITORING_LOCAL_REGISTEREDNAME, | ||
| CONTEXT_MONITOR_ALIAS, | ||
| // Like CONTEXT_MONITOR_MONITORED_LOCAL, but the 'DOWN' carries a process reference (an alias). |
There was a problem hiding this comment.
I find this comment confusing
|
|
||
| term refcopy = term_from_ref_ticks(ref_ticks, &ctx->heap); | ||
|
|
||
| term reply = term_alloc_tuple(3, &ctx->heap); |
There was a problem hiding this comment.
This is a tuple 3 but we alloc for two tuple 2 above.
We had 12 and 2 * TUPLE(2) is 6 I believe, and we need at least 7 in this branch, don't we?
I wonder why we copied the ref and no longer need to. With the ref, the branch was 9. Is there another branch that required 12?
EDIT: Using MAX(TUPLE_SIZE(3), 2 * TUPLE_SIZE(2)) above would avoid my confusion.
| mbox->inner_last = last; | ||
| } | ||
|
|
||
| MailboxMessage *mailbox_process_outer_list(Mailbox *mbox) |
There was a problem hiding this comment.
I am not convinced this works.
There are several calls of mailbox_process_outer_list that haven't been replaced with the _with_alias variant, so Alias is considered as a signal and order is lost. For example get_process_info it seems. This would break message ordering semantics, wouldn't it?
I'm not sure we should have the variant in the first place.
There was a problem hiding this comment.
It seems correct but was confusing. Maybe the alias variant should have the default name and the other should be teardown or native as it's only called for ports, crash dump or destroy.
Implement Erlang process aliases together with the reference-representation
rework they require, rebased onto release-0.7 and squashed into one commit.
Public API (libs/estdlib/src/erlang.erl):
- alias/0, unalias/1
- monitor/3 with {alias, explicit_unalias | demonitor | reply_demonitor}
- spawn_opt {monitor, [monitor_option()]}
Runtime:
- New process-reference term carrying ref_ticks + owner process id (RefData),
alongside short/local references; BEAM-compliant reference ordering and
binary_to_term/term_to_binary round-tripping (term.{c,h}, external_term.c).
- Monitors store RefData; alias monitors (CONTEXT_MONITOR_ALIAS) with the three
alias modes and their lifecycle (context.{c,h}).
- send/2, the send opcode and the JIT send path resolve an alias to its target
process and drop reply_demonitor aliases after first use (nifs.c,
opcodesswitch.h, jit.c).
- Reference size macros: REF_SIZE -> TERM_BOXED_REFERENCE_SHORT_SIZE, plus
TERM_BOXED_REFERENCE_PROCESS_SIZE and TERM_BOXED_REFERENCE_RESOURCE_SIZE.
Tests: alias coverage added to test_monitor, test_refs_ordering and
test_binary_to_term.
Signed-off-by: Davide Bettio <davide@uninstall.it>
0fb9965 to
8ded4a5
Compare
pguyot
left a comment
pguyot
left a comment
There was a problem hiding this comment.
Three small nits, no need to go through another review round IMHO. It's high time we merge this ;-)
| static term nif_erlang_monitor(Context *ctx, int argc, term argv[]) | ||
| { | ||
| UNUSED(argc); | ||
|
|
There was a problem hiding this comment.
nit: we want to drop this empty line now
| if (ref_ticks) { | ||
| int res_size = REF_SIZE + TUPLE_SIZE(2); | ||
| // Reserve before publishing (see above). GC here is safe: new_pid and ref_data are immediates. | ||
| int res_size = TERM_BOXED_REFERENCE_PROCESS_SIZE + TUPLE_SIZE(2); |
There was a problem hiding this comment.
Can you use is_alias to avoid the small memory over-reservation here?
|
|
||
| if (IS_NULL_PTR(target)) { | ||
| int res_size = REF_SIZE + TUPLE_SIZE(5) + target_proc_size; | ||
| int res_size = TERM_BOXED_REFERENCE_PROCESS_SIZE + TUPLE_SIZE(5) + target_proc_size; |
There was a problem hiding this comment.
Can you use is_alias to avoid the small memory over-reservation here?
nif_erlang_monitor locks the target process, then raises badarg when the object type does not match the target, as in monitor(process, Port, ...) or the other way around. That error path returned without releasing the processes table lock, unlike the three sibling out of memory error paths in the same function, leaking the lock and deadlocking the next writer on SMP builds. Unlock the target before raising. Signed-off-by: Davide Bettio <davide@uninstall.it>
The CONTEXT_MONITOR_RESOURCE case fell through to the following monitor cases when the ref ticks did not match. It is benign today, since the next cases only break, but the process alias support added CONTEXT_MONITOR_ALIAS to the same switch, so tighten the fall through. Signed-off-by: Davide Bettio <davide@uninstall.it>
globalcontext_init_process appends every new context to the scheduler waiting queue, but context_destroy never removed it. Destroying a process that was never scheduled, as the spawn NIFs do on option errors and port drivers on initialization failures, left a dangling queue entry pointing into freed memory, corrupting the queue on the next scheduler operation. The defect predates the process alias support; it was first exercised by the spawn_opt atomicity test added later in this series. Dequeue in context_destroy under the processes spinlock, and reset the queue item at the scheduler's own dequeue sites so the destroy time dequeue is a no-op there. A concurrent re-queue cannot happen: scheduler_make_ready refuses Killed and Spawning processes under the same spinlock, and one of these flags is set on every destroy path. Signed-off-by: Davide Bettio <davide@uninstall.it>
Address the issues found while reviewing the process alias support: alias message send races and ordering, monitor and alias lifecycle, out of memory unwinding, reference representation and comparison, and the delivery of alias messages through the mailbox. Complete the API with erlang:alias/1 and with incoming distributed alias sends, and add the missing tests and documentation. Each issue is described and discussed in the pull requests, so the individual findings are not repeated here. See also: GH atomvm#2326, GH atomvm#2027 Signed-off-by: Davide Bettio <davide@uninstall.it>
|
clean new AMP: PR review: process alias supportRange reviewed: Recommendation: Request changes. The alias lifecycle and same-mailbox-batch ordering fixes are generally coherent, and the final monitor-layout change avoids imposing an unnecessary per-monitor memory cost. However, the range still contains one remotely triggerable abort, two broken allocation-failure paths in newly exercised alias/monitor code, and several reference/lifecycle inconsistencies. Findings1. High: malformed distributed alias payloads can abort the VM
For a process-shaped alias, The new branch should validate deserialization just as it validates the control term: diff --git a/src/libAtomVM/dist_nifs.c b/src/libAtomVM/dist_nifs.c
--- a/src/libAtomVM/dist_nifs.c
+++ b/src/libAtomVM/dist_nifs.c
@@ -572,6 +572,9 @@ static term nif_erlang_dist_ctrl_put_data(Context *ctx, int argc, term argv[])
roots[1] = argv[1];
roots[2] = control;
term payload = external_term_from_binary_with_roots(ctx, 1, 1 + bytes_read, &bytes_read, 3, roots);
+ if (UNLIKELY(term_is_invalid_term(payload))) {
+ RAISE_ERROR(BADARG_ATOM);
+ }
control = roots[2];
term target = term_get_tuple_element(control, 2);
if (LIKELY(term_is_process_reference(target))) {Add a distribution regression test with a valid 2. High: monitor creation can return a reference after failing to install the target half
On this failure path:
This contradicts the invariant documented immediately above the call: all fallible work must finish before monitor/alias state is published. The smallest correct fix is contractual rather than a one-line guard: make This path needs allocator fault injection because ordinary monitor tests cannot exercise it deterministically. 3. High: alias-send allocation failure dereferences
|
Address the issues found while reviewing the process alias support: alias message send races and ordering, monitor and alias lifecycle, out of memory unwinding, reference representation and comparison, and the delivery of alias messages through the mailbox. Complete the API with erlang:alias/1 and with incoming distributed alias sends, and add the missing tests and documentation. Each issue is described and discussed in the pull requests, so the individual findings are not repeated here. See also: GH atomvm#2326, GH atomvm#2027 Signed-off-by: Davide Bettio <davide@uninstall.it>
Storing a RefData (ref ticks plus owner pid) in every local monitor grew each MonitorLocalMonitor, MonitorLocalRegisteredNameMonitor and MonitorAlias by 8 bytes (uint64_t alignment), paid by every plain monitor/2, that is by every gen_server:call, on the embedded targets. The pid was redundant everywhere: on the monitored side it duplicates monitor_obj, the monitoring process, which is exactly the alias owner; on the monitoring, registered name and alias entries it was never read. Keep a plain uint64_t ref_ticks in the monitor structs, their baseline layout, and carry the only new bit of information, that the 'DOWN' reference must be rebuilt alias shaped, as a monitor type: CONTEXT_MONITOR_MONITORED_LOCAL_ALIAS. The 'DOWN' path derives the process reference pid from monitor_obj. monitor_new now takes the monitor type directly, replacing the is_monitoring bool as well. RefData stays as the NIF local value type used to build result references (term_from_ref_data) in monitor/3, spawn_opt and alias/1. Behavior is unchanged and pinned by the existing tests: test_monitor_down_alias asserts the 'DOWN' carries the very alias reference, and process_info(monitored_by) keeps listing alias shaped monitors. Signed-off-by: Davide Bettio <davide@uninstall.it>
5 participants
Implement process aliases, so a process can hand out a reference that
works as a send target and deactivate it again once it is done with
it. This is the OTP mechanism behind request/reply patterns that must
not accept a late reply, and AtomVM had no equivalent. The series
rebases the original implementation on
release-0.7and lands thefixes found while reviewing it.
Highlights:
erlang:alias/0,1,erlang:unalias/1,erlang:monitor/3with the{alias, Mode}option, andspawn_opt{monitor, MonitorOpts}.erlang:send/2, the send opcode and theJIT send path, and from a remote node over distribution.
to the ref ticks, with BEAM compliant ordering and
binary_to_term/term_to_binaryround tripping.the owner's monitor list and send order is preserved.
own commits: a leaked process lock in
erlang:monitor/2,3, a switchfall through in
context_demonitor, and a dangling scheduler queueentry left behind by
context_destroy.A send addressed to a remote alias is dropped instead of being routed
over distribution, and the OTP
{tag, Term}monitor option and thealias/1priorityoption raiseunsupported. All three aredocumented.
Supersedes #2027
Closes #2027
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