Do NOT use `async-executor` and older `edge-executor` for now (WAS: embassy-time-driver: Deadlock when timer callback races with executor's concurrent_queue push)

[torkleyy]

TL;DR: Do NOT use async-executor or edge-executor V <= 0.4.1 because the queues they use (crossbeam-queue and concurrent-queue) do NOT work with multi-prio threads and lead to priority inversion).

Until (or if) the problem in async-executor is confirmed and fixed, consider using:

• embassy-executor
• edge-executor from GIT
• The executor from the futures crate

See this for more details.

====================================

I've been hitting intermittent deadlocks when using the embassy-time-driver feature with async-executor. After a lot of funny watchdog resets, firmware hangs and jtag debugging, I was able to trace it back to a priority inversion between the ESP-IDF timer task and the async executor.

The Problem

The timer callback in embassy_time_driver runs on the ESP-IDF timer task (high priority) and directly calls schedule_next_expiration(), which eventually invokes waker.wake() for expired timers.

These wakers originate from async-executor, which internally uses concurrent_queue for its task queue.

When the executor (running at normal priority) is in the middle of a concurrent_queue::push() operation and a timer fires, the following sequence occurs:

1. The executor is mid-push to concurrent_queue (at a block boundary in the queue's internal state).
2. The timer task preempts and runs the callback.
3. The callback calls waker.wake(), which attempts to push to the same concurrent_queue.
4. concurrent_queue detects contention and calls busy_wait(), which invokes std::thread::yield_now().
5. On FreeRTOS, yield_now() maps to vTaskDelay(0), which only yields to equal or higher priority tasks.
6. The lower-priority executor never resumes execution, resulting in a deadlock.

Relevant Code

esp-idf-svc/src/timer.rs

Lines 484 to 489 in 71bb200

	.timer(move || {
	static_self
	.inner
	.lock()
	.borrow_mut()
	.schedule_next_expiration()

The schedule_next_expiration() call chains into embassy_time_queue_utils::Queue::next_expiration(), which calls waker.wake() for each expired timer.

Environment

• ESP32 (dual-core; reproduces on single-core as well)
• async-executor as the runtime
• Heavy timer usage (multiple concurrent Timer::after() calls)

Potential Fix Direction

The timer callback should not call waker.wake() directly, since it executes at high priority. A possible approach:

• The timer callback signals a semaphore and returns immediately.
• A helper task, running at the same priority as the executor, waits on the semaphore and calls schedule_next_expiration().
• With equal priorities, vTaskDelay(0) should correctly yield between tasks.

Not sure if there's a simpler fix, if you have an idea please let me know...
I think this is the same problem as #619

[ivmarkov]

@torkleyy Sorry for the delay, I've been thinking about this on and off for a while. And thanks for diagnosing!

I wonder whether std::thread::yield_now() is actually free of priority inversion problems even on "real" STD platforms like Linux, where you could still have threads with different priorities?

Because if it is not, one could argue that the way concurrent-queue is designed is a bit flawed to begin with?

An easy workaround would be to have the ESP-IDF timer task running with a lower priority than the async executor task/thread? But this is just not nice...

[ivmarkov]

And yes, your guess that #619 is related might be correct, because in edge-executor we are using crossbeam-queue by default, and it seems to be affected by the same problem: https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-queue/src/array_queue.rs#L177

[ivmarkov]

@torkleyy Any feedback?

Because if it is not, one could argue that the way concurrent-queue is designed is a bit flawed to begin with?

I think my assessment is correct ^^^ no? Shall we open a bug at their repo?

[torkleyy]

Yes, it seems to me that both concurrent-queue and crossbeam-queue make the assumption that tasks run at equal priorities, otherwise it's incorrect.

I implemented a workaround in #637

[torkleyy]

Actually, the same problem is present in async-executor, which can lead to deadlocks in the waker...

[ivmarkov]

Actually, the same problem is present in async-executor, which can lead to deadlocks in the waker...

I thought we were talking about async-executor and edge-executor all-along, as these two use the concurrent-queue and crossbeam-queue crates which have the yield_now problem?

If you could clarify?

[torkleyy]

Sorry for the confusion. Yes, you're right, I only realized just now that this problem is not limited to timers at all.

[torkleyy]

This is working well enough for me: https://github.com/torkleyy/freertos-executor
Essentially it solves the problem by using a freertos queue which has priority inversion.

[ivmarkov]

This is working well enough for me: https://github.com/torkleyy/freertos-executor Essentially it solves the problem by using a freertos queue which has priority inversion.

@torkleyy I wonder whether the latest master of edge-executor would also work for you?
(Sorry for pushing directly to edge-executor master rather than a PR - should remember to drink my morning coffee first.)

It is not really using a FreeRTOS-specific queue, but I don't think this is necessary either. As long as the queue is using the OS-native mutexes rather than spinlocks, the priority inversion problem should be gone.

The two queue implementations are generified by the RawMutex type, so you can just use the default CriticalSectionRawMutex, or even this one if you need an ISR-safe executor (which I hope you don't).

EDIT: But you should be capable of plugging-in a FreeRTOS-specific queue as well. Note that we even have wrappers for the FreeRTOS/ESP-IDF queue in esp-idf-hal My personal issue with the FreeRTOS queue is that it is bounded YET it still requires allocations, i.e. you potentially get the latency of the ESP-IDF TLSF allocator kicking in in tight loops like awaking a waker.