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.
Michael Stack