The original `ThreadReturnPromiseStream` had a fundamental flaw: it didn't handle reference counting
in a thread-safe way. This led to subtle and hard-to-debug race conditions, especially since Future
and Promise often live on different threads in this use case.
This patch introduces a new implementation of ThreadReturnPromiseStream that uses an atomics backed
spinlock to safely manage reference counts. As a result, the Flow compiler also needs to support a
new type: `ThreadFutureStream`, in addition to the existing `FutureStream`.
The underlying `NotifiedQueue` now has a parallel implementation called `ThreadNotifiedQueue` to
make this possible. The higher-level thread-safe promise and promise counterpart are built on top of
that.
The new `ThreadFutureStream` is compatible with both Flow and C++ coroutines. The original
non-threaded Flow primitives remain untouched and free from the overhead of atomics.
Joshua:
20250417-000356-vishesh-16ce18efeab40158 compressed=True data_size=41124943 duration=5436274 ended=99998 fail=1 fail_fast=10 max_runs=100000 pass=99997 priority=100 remaining=0:00:00 runtime=1:00:47 sanity=False started=100000 submitted=20250417-000356 timeout=5400 username=vishesh
clang-format
We crash with rocksdb storage when bulkloading at scale
with this change in place. Removing for now.
This reverts commit 55edaf5ed1.
Co-authored-by: stack <stack@duboce.com>
`getFuture()` should be called before post as `send`/`sendError`
operation in `ThreadReturnPromise` moves the underlying Promise to
`tagAndForward()`.
Ideally, `ThreadReturnPromise` behavior should stay consistent with the
`Promise`. However, the problem is that it relies on the invariant that
there will always be one owner of its internal `Promise` which is either
itself or `tagOrForward` -- which is necessary to ensure that only one
thread can operate on the Promise's internal state (ref count, flags
etc) and avoid race conditions.
This patch (1) makes sure that in case of `post()` function we get
future before, (2) adds an ASSERT as this should never happen, (3)
documentation for future users and (4) a test case for potentially
fixing this in future.
When a value/error is sent via `ThreadReturnPromiseStream` we assume that the underlying
`PromiseStream` will be alive when the client waits. However, if the last
`ThreadReturnPromiseStream` gets destroyed after sending values/end_of_stream(), the underlying
`PromiseStream` will as well resulting in `broken_promise`. This happens because the actual work of
sending the value/error is deferred on the main thread.
This is likely to happen because the sender did its work and it isn't supposed to check if client
got the value. Hence, little reason to keep the promise. Meanwhile, client is free to read values
from its future whenever it needs to.
This patch just holds the reference to underlying `NotifiedQueue` by copying `PromiseStream` until
the value/error is sent. The test added would fail without this patch.
