Skip to content

fix: SettableAsyncResource clear callbacks if closed before set - #20074

Draft
clintropolis wants to merge 2 commits into
apache:masterfrom
clintropolis:fix-settable-async-callback
Draft

fix: SettableAsyncResource clear callbacks if closed before set#20074
clintropolis wants to merge 2 commits into
apache:masterfrom
clintropolis:fix-settable-async-callback

Conversation

@clintropolis

Copy link
Copy Markdown
Member

Description

Fixes SettableAsyncResource to adhere to the contract of javadoc of AsyncResource#addReadyCallback where callbacks are not meant to be fired if close is called before set/setException. Not sure if this is causing any bugs, but plausible?

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Severity Findings
P0 0
P1 1
P2 1
P3 0
Total 2

Reviewed 3 of 3 changed files.

The review found one lifecycle hang and one cancellation-log regression.


This is an automated review by Codex GPT-5.6-Luna(max)

// close() was called before set/setException.
result = null;
canceler = null;
readyCallbacks.clear();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Closing can strand await callers

AsyncResource.await() registers a latch callback and blocks. Clearing callbacks on close removes that wake-up path; if close wins before completion, a late set or setException drains nothing, so an unbounded await blocks indefinitely and a timed await waits until its timeout. Add a close-aware wake-up path and a regression test.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is ok, the consumer is expected to call close to abandon an acquire, while the producer should only ever call set/setException, meaning that this should only be a problem in cases where there are multiple consumer threads I think? Nothing is using await in production currently, but added some javadoc to try to clear up the usage model.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 changed files.

The Javadocs clarify the intended usage model, but the lifecycle race remains: await() relies on a callback that close() now removes. An independent cancellation thread can therefore strand a single consumer in await() indefinitely. Please add a close-aware wake-up path and regression test.

@capistrant capistrant left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for adding clarification in javadocs to help potential future callers understand expected use

@FrankChen021 FrankChen021 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 changed files for this follow-up.

The await lifecycle concern remains and was addressed in an inline reply; the prior logging concern is resolved by debug-only logging. No additional inline findings.


This is an automated review by Codex GPT-5.6-Luna(max)

@clintropolis
clintropolis marked this pull request as draft August 21, 2026 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants