Skip to content

fix: prevent duplicate phantom-reachable cleanup - #885

Open
sylvesterkaczmarek wants to merge 1 commit into
openai:mainfrom
sylvesterkaczmarek:fix/phantom-close-once
Open

fix: prevent duplicate phantom-reachable cleanup#885
sylvesterkaczmarek wants to merge 1 commit into
openai:mainfrom
sylvesterkaczmarek:fix/phantom-close-once

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown

Summary

Make explicit close and phantom-reachable fallback cleanup share one at-most-once action for the SDK's closable phantom wrappers.

Fixes #884.

Problem

closeWhenPhantomReachable(...) currently registers a Java 9+ Cleaner action and discards the registration handle. The closable wrappers then implement explicit close() by invoking the underlying resource independently.

That leaves two active cleanup paths:

  1. explicit wrapper close;
  2. the Cleaner action when the wrapper (or async reachability tracker) is later collected.

A delegate can therefore be closed twice. AutoCloseable.close() is not generally required to be idempotent, and provider-owned/custom resources can perform stateful cleanup or throw on a repeated close.

Fix

closeWhenPhantomReachable(...) now:

  • creates one cleanup closure guarded by AtomicBoolean.compareAndSet(false, true);
  • registers that same guarded closure with the Cleaner when available;
  • returns an AutoCloseable handle that invokes the same closure explicitly.

The five closable phantom wrappers retain that handle and use it from close():

  • PhantomReachableSleeper
  • PhantomReachableClosingHttpClient
  • PhantomReachableClosingStreamResponse
  • PhantomReachableClosingAsyncStreamResponse
  • PhantomReachableClosingHttpRequestAuthenticator

The Java 8 behavior remains compatible: there is still no GC Cleaner, while the returned handle performs explicit cleanup through the same once-guard.

PhantomReachableExecutorService is intentionally unchanged because its lifecycle API is shutdown/shutdownNow rather than AutoCloseable, and repeated shutdown() has different semantics.

Regression coverage

  • preserves the existing GC-triggered cleanup test;
  • verifies the explicit close handle runs cleanup at most once;
  • verifies PhantomReachableClosingHttpClient.close() closes its delegate at most once across repeated explicit calls.

Validation

  • branch is based directly on current upstream main at cf942a40074291290634321ad9fe21e514030b4c;
  • branch is 0 commits behind upstream;
  • implementation is a single commit;
  • shared helper production diff is 15 additions / 4 deletions;
  • each wrapper change is limited to retaining the returned handle and using it for explicit close.

Full repository validation is left to GitHub Actions.

Risk

Low. Forgotten resources still receive Cleaner-based fallback cleanup. Explicitly closed resources now suppress the later duplicate cleanup instead of relying on every delegate to tolerate being closed twice.

@sylvesterkaczmarek
sylvesterkaczmarek requested a review from a team as a code owner August 18, 2026 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Phantom-reachable wrappers can close resources twice after explicit cleanup

1 participant