Skip to content

fix: run all HBaseIO cleanup steps even if an earlier close() throws [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT] - #39711

Open
waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/hbaseio-close-leak
Open

fix: run all HBaseIO cleanup steps even if an earlier close() throws [fj4WqyCCw3C5ShR1RfB7MoBPTpkRrBFYP1uT35g3MvT]#39711
waterWang wants to merge 1 commit into
apache:masterfrom
waterWang:fix/hbaseio-close-leak

Conversation

@waterWang

Copy link
Copy Markdown

Fixes #39710

What

HBaseIO releases its resources as consecutive, unguarded statements in three teardown paths. If an earlier close() throws, everything after it is skipped:

  1. HBaseReader.close() — a throwing scanner.close() leaks the Connection.
  2. HBaseWriterFn.tearDown()BufferedMutator.close() is documented to throw on a final flush failure (down region server, network blip), so a failed flush skips connection.close(), leaking the whole per-@Setup Connection.
  3. HBaseRowMutationWriterFn.tearDown() — worst case: a throwing table.close() skips HBaseSharedConnection.close(configuration), which is a reference-count decrement. Because the pool is a static HashMap, the leaked entry (and its ZooKeeper session / RPC threads) survives for the JVM lifetime and every later getOrCreate hands back the same permanently-unreleasable connection.

Fix

Wrap each cleanup step in try/finally so every step runs unconditionally, while preserving the first failure and attaching later ones with addSuppressed, so callers still see the error that actually broke the job rather than a teardown symptom.

  • HBaseReader.close(): connection is now always closed even if scanner.close() throws.
  • HBaseWriterFn.tearDown(): connection.close() always runs even if mutator.close() throws.
  • HBaseRowMutationWriterFn.tearDown(): HBaseSharedConnection.close(configuration) (the refcount decrement) always runs even if table.close() throws.

Testing

Existing tests in sdks/java/io/hbase/src/test/java/org/apache/beam/sdk/io/hbase/ cover the read and write paths; the change is purely control-flow (guarding cleanup), so no new behavior is introduced when nothing throws.

@PDGGK

PDGGK commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@waterWang heads-up on an overlap, and an offer.

I filed #39710 and then opened #39712 for it without re-checking — yours went up ten minutes before mine and I simply missed it. That's on me. Yours is first on the clock and the fix reads correct to me: same three sites, same "run every step, keep the first failure, suppress the rest" shape, and you got the HBaseSharedConnection.close() ordering right in the third one, which is the case that actually matters.

The one thing yours doesn't have is tests, and CI is currently showing 2 failures. Mine carries HBaseIOCloseTest — 5 cases:

  • the two failure-collection helpers
  • one per teardown, each asserting the later resource is still released
  • the row-mutation one asserts HBaseSharedConnection.getConnectionCount() actually returns to zero, rather than verifying a mock, since the stranded refcount is the real consequence

Reverting each teardown one at a time fails exactly and only its own test, so they're pinned per-site rather than as a block.

You're welcome to take that file wholesale — it's at HBaseIOCloseTest.java in #39712; copy it in and I'll close mine. It needs testImplementation library.java.mockito_core added to sdks/java/io/hbase/build.gradle and three nested classes dropped from private to package-private so the test can construct them. If you'd rather not, that's fine too — I've said on my side that a committer should just pick one.

No ownership claim here, I'd just like the fix to land with coverage.

@github-actions

Copy link
Copy Markdown
Contributor

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

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.

[Bug]: HBaseIO skips connection cleanup when an earlier close() throws, permanently corrupting the shared-connection refcount

2 participants