coroutine: allocate coroutine frame in a critical section #2900
Conversation
|
Helps #2899 |
There was a problem hiding this comment.
Pull Request Overview
This PR allocates coroutine frames within a critical section to prevent memory allocation failures from causing system instability. The change treats coroutine frame allocation similarly to future continuation allocation, where failure is not an option since it would leave the system in an undefined state.
- Adds custom
operator newto coroutine promise types that usesmemory::scoped_critical_alloc_section - Prevents memory allocation failure injection from interfering with coroutine frame allocation
- Resolves conflicts with test code that injects memory failures to test error handling
include/seastar/core/coroutine.hh
Outdated
|
|
||
| static void* operator new(size_t size) { | ||
| memory::scoped_critical_alloc_section _; | ||
| return ::operator new(size + sizeof(promise_type)); |
There was a problem hiding this comment.
The size calculation is incorrect. The size parameter already includes the size of the coroutine frame which contains the promise_type. Adding sizeof(promise_type) will result in over-allocation and potential memory corruption.
| return ::operator new(size + sizeof(promise_type)); | |
| return ::operator new(size); |
Copilot uses AI. Check for mistakes.
include/seastar/core/coroutine.hh
Outdated
|
|
||
| static void* operator new(size_t size) { | ||
| memory::scoped_critical_alloc_section _; | ||
| return ::operator new(size + sizeof(promise_type)); |
There was a problem hiding this comment.
The size calculation is incorrect. The size parameter already includes the size of the coroutine frame which contains the promise_type. Adding sizeof(promise_type) will result in over-allocation and potential memory corruption.
| return ::operator new(size + sizeof(promise_type)); | |
| return ::operator new(size); |
Copilot uses AI. Check for mistakes.
avikivity
force-pushed
the
coroutine-critical-section
branch
from
23fce6d to
d4a5009
Compare
|
v2: don't over-allocate (bad copilot) |
Like continuations, coroutines are glue that cannot be allowed to fail. For example, if cleanup coroutine fails to allocate and returns an exception future, cleanup will not be done and the system will enter an undefined state. Better to crash. This conflicts with test code that tries to inject memory failures and see how they are handled (file_io_test.cc handle_bad_alloc_test). The coroutine will throw std::bad_alloc (since we don't define get_return_object_on_allocation_failure()), and since it's declared noexcept, will call std::terminate. To avoid all this, follow future::schedule() and prevent memory allocation failure injection from interfering with coroutine frame allocation. In this regard a coroutine frame is just like a continuation. This is done by injecting class-specific operator new and operator delete. Sized deallocation is also defined if supported by the compiler.
avikivity
force-pushed
the
coroutine-critical-section
branch
from
d4a5009 to
d91b2db
Compare
|
v3:
|
|
This breaks code that previously handled future allocation errors correctly. |
|
Futures don't allocate. |
So why you added operator new/ operator delete in this pull request? |
Because coroutine frames are allocated. |
2 participants
Like continuations, coroutines are glue that cannot be allowed to fail. For example, if cleanup coroutine fails to allocate and returns an exception future, cleanup will not be done and the system will enter an undefined state. Better to crash.
This conflicts with test code that tries to inject memory failures and see how they are handled (file_io_test.cc handle_bad_alloc_test). The coroutine will throw std::bad_alloc (since we don't define get_return_object_on_allocation_failure()), and since it's declared noexcept, will call std::terminate.
To avoid all this, follow future::schedule() and prevent memory allocation failure injection from interfering with coroutine frame allocation. In this regard a coroutine frame is just like a continuation.