Process aliases#2027
Conversation
e2c4216 to
f30b1f3
Compare
|
@bettio it seems that CI sometimes fails because it cannot find |
| #define BOXED_FUN_SIZE 3 | ||
| #define FLOAT_SIZE (sizeof(float_term_t) / sizeof(term) + 1) | ||
| #define REF_SIZE ((int) ((sizeof(uint64_t) / sizeof(term)) + 1)) | ||
| #define TERM_BOXED_PID_REF_SIZE (REF_SIZE + 1) |
There was a problem hiding this comment.
For consistency with the work we are doing lately I suggest renaming all PID_REF and pid_reference to PROCESS_REF and process_reference
| term *boxed_value = memory_heap_alloc(heap, TERM_BOXED_PID_REF_SIZE); | ||
| boxed_value[0] = TERM_BOXED_PID_REF_HEADER; | ||
|
|
||
| #if TERM_BYTES == 8 |
There was a problem hiding this comment.
nitpicking/very low priority comment: just for consistency I would suggest moving first the 4 branch:
#if TERM_BYTES == 4 ... #elif TERM_BYTES == 8.
Same applies to the other functions as well.
| return ret; | ||
|
|
||
| } else if (term_is_pid_reference(t)) { | ||
| int32_t process_id = term_pid_ref_to_process_id(t); |
There was a problem hiding this comment.
We are using uint32_t for process id.
There was a problem hiding this comment.
Are you sure? It's int32 here: https://github.com/atomvm/AtomVM/blob/main/src/libAtomVM/context.h#L106 and in many other places
| ok = test_alias(), | ||
| ok = test_monitor_alias(), | ||
| ok = test_monitor_alias_explicit_unalias(), | ||
| ok = test_monitor_down_alias(), |
There was a problem hiding this comment.
We should also test unhappy paths, such as:
- double/triple unalias
- alias a dead process
- unalias a dead process
In addition multiple aliases should be tested, this is legit but is should be properly tested.
| RAISE_ERROR(BADARG_ATOM); | ||
| } | ||
|
|
||
| while (!term_is_nil(options)) { |
There was a problem hiding this comment.
We should use term_is_nonempty_list to avoid issues with non proper lists.
| RAISE_ERROR(UNSUPPORTED_ATOM); | ||
| } else { | ||
| RAISE_ERROR(BADARG_ATOM); | ||
| } |
There was a problem hiding this comment.
here we check that last list item is nil.
| enum ContextMonitorAliasType | ||
| { | ||
| CONTEXT_MONITOR_ALIAS_EXPLICIT_UNALIAS, | ||
| CONTEXT_MONITOR_ALIAS_DEMONITOR, |
There was a problem hiding this comment.
Let's use PascalCase. See AVMCCS-N004 rule in our coding style guide.
(There is still some old code that has to be migrated to updated style).
4b23228 to
07373d0
Compare
|
@bettio added more tests and missing features. The CI is failing, but it seems unrelated to the changes |
bettio
left a comment
There was a problem hiding this comment.
Looks mostly good: there is only one change related to IS_NULL_PTR that is required. I suggest also rebasing on top of main, since main has a lot of fixes for flaky tests.
| Context *p = globalcontext_get_process_lock(glb, local_process_id); | ||
| if (p) { | ||
| struct MonitorAlias *alias = context_find_alias(p, ref_ticks); | ||
| if (!IS_NULL_PTR(alias)) { |
There was a problem hiding this comment.
We should not use !IS_NULL_PTR (x) since it is a shorthand for UNLIKELY(x == NULL). So we must use it only when it is unlikely that the pointer is null (that is mostly for error handling purposes).
There was a problem hiding this comment.
Done, but there are many occurrences of !IS_NULL_PTR in the code
92db4b7 to
1bdef14
Compare
|
esp32 tests are crashing |
pguyot
left a comment
There was a problem hiding this comment.
My main concern so far is that monitors are now extended to 2 or 3 words instead of 1 or 2. This may break some code that took monitors and passed it around as references, as the term_to_ref_ticks/term_from_ref_ticks is a common pattern. A similar breakage existed with resources being larger references if one tries to pass a resource where a 1/2 words ref is expected, but it is less likely as resources were not references so far. This may be the crash cause of esp32 tests.
port.erl
call(Port, Message, Timeout) ->
MonitorRef = monitor(port, Port),
Port ! {'$call', {self(), MonitorRef}, Message},
Result =
receive
{'DOWN', MonitorRef, port, Port, normal} ->
{error, noproc};
{'DOWN', MonitorRef, port, Port, Reason} ->
{error, Reason};
out_of_memory ->
out_of_memory;
{MonitorRef, Ret} ->
Ret
after Timeout ->
{error, timeout}
end,
demonitor(MonitorRef, [flush]),
Result.
uart_driver.c:
static void uart_driver_do_read(Context *ctx, GenMessage gen_message)
{
GlobalContext *glb = ctx->global;
struct UARTData *uart_data = ctx->platform_data;
term pid = gen_message.pid;
term ref = gen_message.ref;
uint64_t ref_ticks = term_to_ref_ticks(ref);
| -type raise_stacktrace() :: | ||
| [{module(), atom(), arity() | [term()]} | {function(), arity() | [term()]}] | stacktrace(). | ||
|
|
||
| -type monitor_option() :: {'alias', 'explicit_unalias' | 'demonitor' | 'reply_demonitor'}. |
There was a problem hiding this comment.
This is not enforced by style guide but usually we don't quote atoms when it's not required.
| } | ||
| // Prepare the message on ctx's heap which will be freed afterwards. | ||
| term ref = term_from_ref_ticks(monitored_monitor->ref_ticks, &ctx->heap); | ||
| term ref = term_make_process_reference(target->process_id, monitored_monitor->ref_ticks, &ctx->heap); |
There was a problem hiding this comment.
This is not what OTP does and there probably is a good reason.
Regular monitors are smaller than aliases.
7> Pid = spawn(fun() -> receive ok -> ok end end).
<0.94.0>
8> monitor(process, Pid).
#Ref<0.3008598808.1200095248.57512>
9> monitor(process, Pid, [{alias, explicit_unalias}]).
#Ref<0.0.11651.3008598808.1200160784.57533>
10> make_ref().
#Ref<0.3008598808.1200095248.57553>
| boxed_value[0] = TERM_BOXED_PROCESS_REF_HEADER; | ||
|
|
||
| #if TERM_BYTES == 4 | ||
| boxed_value[1] = (ref_ticks >> 4); |
There was a problem hiding this comment.
This was fixed for regular refs, we probably want ref_ticks >> 32
| end, | ||
| Term. | ||
|
|
||
| is_atomvm_or_otp_version_at_least(OTPVersion) -> |
There was a problem hiding this comment.
get_otp_version/0 already exists in this module.
Also there is a poor usage (fixed in #2025 )
OTPVersion = get_otp_version(),
if
OTPVersion =:= atomvm orelse OTPVersion >= 26 ->
B == erlang:term_to_binary(A);
true ->
true
end.
be sure to rather do:
OTPVersion = get_otp_version(),
if
OTPVersion >= 26 ->
B == erlang:term_to_binary(A);
true ->
true
end.
as atoms sort higher than integers.
There was a problem hiding this comment.
Also there is a poor usage
I would call it 'a readable way' :P
ce57282 to
eac7682
Compare
|
@pguyot the reason for the STM32 tests failing was that the size of a regular reference is actually 3 terms there, which means process reference takes 4 terms, and it probably conflicts with resource reference size, which is always 4 terms. I changed the process reference size to 5, but that's not a perfect solution :P So we need to come up with another way of distinguishing references - do you have anything in mind? I also added |
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
31b3f98 to
f836ea9
Compare
f836ea9 to
8651323
Compare
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
|
Amp delivered this review: https://ampcode.com/threads/T-019cc41b-47c0-735d-a474-3fc46fe3caee - expanded here: https://gist.github.com/petermm/c3286d0cd622e7d5985a32dbca4b006d All caveats apply - and I'm fully pragmatic about llm being both wrong and pedantic - so pick and choose, and let's land this sooner rather than later, even if there is an todo issue - with further hardening.. PR #2027 Review — Process Aliases & Process ReferencesBranch: SummaryThis PR adds Erlang/OTP-compatible process aliases to AtomVM. Key changes:
The design direction is sound and the test coverage is good. However, there are several issues that should be addressed before merge. 🔴 Critical Issues1.
|
Signed-off-by: Mateusz Front <mateusz.front@swmansion.com>
c98486e to
3b126b0
Compare
|
From the AI review above, the only real issue I see is
@bettio is that true? What's the best way to fix it? |
|
I LLM progressed the thread safety issue here: https://gist.github.com/petermm/f361a5cd73314f890b9e9983593252a9
and commits here https://github.com/petermm/AtomVM/commits/fix/alias-message-thread-safety/ but we probably need som guidance if this is the direction @bettio - this is my first time diving into these areas of AtomVM.. |
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>
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>
Add support for process aliases
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.7` and lands the
fixes found while reviewing it.
Highlights:
- `erlang:alias/0,1`, `erlang:unalias/1`, `erlang:monitor/3` with the
`{alias, Mode}` option, and `spawn_opt` `{monitor, MonitorOpts}`.
- Sending to an alias from `erlang:send/2`, the send opcode and the
JIT send path, and from a remote node over distribution.
- A new process reference term that carries the owner process id next
to the ref ticks, with BEAM compliant ordering and
`binary_to_term`/`term_to_binary` round tripping.
- Alias sends are posted as a mailbox signal, so a sender never walks
the owner's monitor list and send order is preserved.
- Three pre-existing defects surfaced along the way are fixed in their
own commits: a leaked process lock in `erlang:monitor/2,3`, a switch
fall through in `context_demonitor`, and a dangling scheduler queue
entry 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 the
`alias/1` `priority` option raise `unsupported`. All three are
documented.
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
|
Merged #2326 |
TODO:
spawn_optThese 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