[gabrielmancini]

globalcontext_send_message_from_task pushes onto the head of the global message queue when it cannot enqueue directly, and globalcontext_process_message_queue drained it from the head, so messages a task sent in a row reached the mailbox in reverse order.

Without SMP every message from a task goes through this queue. Found on the ESP32-C3 in QEMU while testing Ethernet (#2419): the network driver posts eth_started then eth_connected, and the network process received them the other way round, so network:eth_status/0 stayed started. With SMP the same happens whenever the process table or the processes spinlock is busy.

The fix reverses the queue before enqueuing. test-mailbox gets a test that sends 1, 2, 3 from a task with the spinlock held (which forces the queued path with SMP too) and checks they arrive in that order; it fails without the fix.

Tested: test-mailbox, test-structs and test-erlang on generic_unix (ASan); the ESP32-C3 QEMU suite of #2419 passes with it.

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

[pguyot]

test_send_message_from_task_order and its call may need to be guarded with AVM_TASK_DRIVER_ENABLED

[petermm]

ran into same bug just now, just need the test guards afaik:

Review: AtomVM PR #2423

PR: Fix order of messages sent from tasks
— reviewed head.

Verdict: correct runtime fix; guard the test before merging. Reversing the
detached queue restores deferred-message publication order without new allocation
or locking. Concurrent producers append to the next batch, not the detached list.

Required: support task-driver-disabled builds

globalcontext_send_message_from_task and queue processing are only available
with AVM_TASK_DRIVER_ENABLED. The new test and its call need the same guard
(also raised in the existing review).

--- a/tests/test-mailbox.c
+++ b/tests/test-mailbox.c
@@
+#ifdef AVM_TASK_DRIVER_ENABLED
 void test_send_message_from_task_order(void)
 {
@@
     context_destroy(ctx);
     globalcontext_destroy(glb);
 }
+#endif
 
 int main(int argc, char **argv)
@@
     test_mailbox_send();
     test_mailbox_next();
+#ifdef AVM_TASK_DRIVER_ENABLED
     test_send_message_from_task_order();
+#endif

Suggested test: preserve order across successive drains

The current test covers one batch. Queue another before consuming the mailbox
and require 1..6; this also checks that a later drain cannot overtake an earlier
one. Insert after the existing queue-processing call:

@@ test_send_message_from_task_order
     globalcontext_process_task_driver_queues(glb);
+#ifndef AVM_NO_SMP
+    smp_spinlock_lock(&glb->processes_spinlock);
+#endif
+    for (int i = 4; i <= 6; i++) {
+        globalcontext_send_message_from_task(glb, ctx->process_id, NormalMessage, term_from_int(i));
+    }
+#ifndef AVM_NO_SMP
+    smp_spinlock_unlock(&glb->processes_spinlock);
+#endif
+    globalcontext_process_task_driver_queues(glb);
 
     assert(mailbox_process_outer_list_native(&ctx->mailbox) == NULL);
-    for (int i = 1; i <= 3; i++) {
+    for (int i = 1; i <= 6; i++) {

Scope and evidence

• Not a complete SMP ordering guarantee: a later direct send can bypass an
  earlier deferred send. This is pre-existing and need not block this scoped fix.
• Local equivalent: the two-batch regression fails before the fix and passes
  afterward; mailbox, ENIF, heap, term and structs host tests pass. A separate
  non-SMP host run aborts in pre-existing generic_unix signal initialization,
  before reaching the new test.

[gabrielmancini]

Thanks both. Pushed c784075: the test and its call are under AVM_TASK_DRIVER_ENABLED, and it now sends two batches (1..3, then 4..6) before reading the mailbox and expects 1..6.

Checked on generic_unix: it passes with the fix and fails without it (ASan build), and it builds and passes with -DAVM_DISABLE_TASK_DRIVER=ON and with -DAVM_DISABLE_SMP=ON.

[bettio]

We are super close to merging this PR.

I suggest also squashing the 2 commits of this PR into a single one: the first commit fails to build with -DAVM_DISABLE_TASK_DRIVER=ON.

[gabrielmancini]

Thanks. Squashed into a single commit (761e200, force-pushed) with both suggestions:

• send_from_task takes the lock with SMP_SPINLOCK_LOCK/SMP_SPINLOCK_UNLOCK, without the #ifndef AVM_NO_SMP block.
• assert(glb->message_queue != NULL) before the drain, so the test fails if the messages ever take the direct path.

test-mailbox passes with ASan on the default build, with -DAVM_DISABLE_SMP=ON and with -DAVM_DISABLE_TASK_DRIVER=ON (where the test is not built), and still fails without the fix in globalcontext.c (i == term_to_int(t)). clang-format 16 is clean on the file.