kita-renji commented on code in PR #51498:
URL: https://github.com/apache/arrow/pull/51498#discussion_r4118914081
##########
cpp/src/arrow/util/async_generator.h:
##########
@@ -1338,8 +1363,11 @@ class MergedGenerator {
// Now we have given up the lock and we can take all the actions we
decided we
// need to take.
+ if (signaled_error) {
+ RunErrorSignaledHookForTesting();
+ }
if (should_mark_final_error) {
- state->MarkFinalError(maybe_next->status(), std::move(sink));
+ state->DeliverFinalError(maybe_next->status(), std::move(sink));
Review Comment:
Pushed 364639c41, which stops relying on callbacks for ordering altogether:
- `all_finished` is gone. `MarkFinishedAndPurge` completes the remaining
futures itself, in the order they were handed out: the future receiving the
error first (a waiter, or the first caller to ask after an error nobody was
waiting for), then every waiting caller and every terminal item requested
since, which get `IterationEnd`. Terminal pulls made after the generator is
broken or exhausted are queued in `waiting_jobs` instead of being chained on
`all_finished`. Pulls that arrive while the queue is being completed are queued
behind it, and it is drained until it stays empty.
- A caller that asks from the callbacks of the last pending future still
gets an already-finished `IterationEnd`, as it did before, so a callback that
pulls and then blocks cannot deadlock.
- While testing this I found that the same mechanism also let a later
`IterationEnd` complete before earlier ones: all terminal pulls hung off
`all_finished`, so one added mid-dispatch ran at once. That's fixed by the same
change.
Tests:
- `{Inner,Outer}ErrorToWaiterNotOvertakenDuringCompletion` and
`ClaimedErrorNotOvertakenDuringCompletion` use your technique: hold the earlier
future's mutex via `TryAddCallback` and pull once the generator has started
completing. All three fail every time on the previous commit.
- `PullFromLastFutureCallbackCompletesAtOnce`.
- `MergedGeneratorStressTest.TerminalNeverOvertakesEarlierFutures` checks
the contract directly with inner and outer failures on the thread pool: no
terminal completes while an earlier future is pending, and exactly one error is
delivered.
All 138 tests in `arrow-async-utility-test` pass. The `MergedGenerator`
tests pass 300/300 repeats in debug and 30/30 under TSAN with no reports.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
