You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@bookkeeper.apache.org by "kroosxu (via GitHub)" <gi...@apache.org> on 2026/05/25 12:22:57 UTC
[PR] Issue 4797:Prevent thread leak when BookKeeper client constructor fails [bookkeeper]
kroosxu opened a new pull request, #4798:
URL: https://github.com/apache/bookkeeper/pull/4798
Descriptions of the changes in this PR:
Fix #4797
### Motivation
When `BookKeeper.forConfig(conf).build()` fails part-way through construction
(for example because `metadataServiceUri` points to an unreachable or invalid
ZooKeeper ensemble, or `MetadataDrivers#getClientDriver(URI)` cannot resolve
a driver for the configured scheme), the constructor leaks every resource it
has already allocated up to the point of failure.
### Changes
- In the `BookKeeper` constructor, when initialization fails *after* the
worker pools / bookie client have been created but *before* the object is
fully constructed, invoke `close()` on the partially-initialized instance
so that all already-allocated resources are
shut down before the exception is propagated to the caller.
- Add `BookKeeperConstructorFailureTest` under
`bookkeeper-server/src/test/java/org/apache/bookkeeper/client/` covering
the regression:
- `testNoThreadLeakWhenMetadataServiceUnavailable` — a single failing
`build()` must not leave any client-owned thread behind.
- `testRepeatedFailedBuildsDoNotAccumulateThreads` — 20 consecutive
failing `build()` calls must not accumulate threads.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4797:Prevent thread leak when BookKeeper client constructor fails [bookkeeper]
Posted by "wenbingshen (via GitHub)" <gi...@apache.org>.
wenbingshen merged PR #4798:
URL: https://github.com/apache/bookkeeper/pull/4798
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4797:Prevent thread leak when BookKeeper client constructor fails [bookkeeper]
Posted by "kroosxu (via GitHub)" <gi...@apache.org>.
kroosxu commented on code in PR #4798:
URL: https://github.com/apache/bookkeeper/pull/4798#discussion_r3681266606
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -459,9 +459,11 @@ public BookKeeper(ClientConfiguration conf, ZooKeeper zk, EventLoopGroup eventLo
log.error()
.exception(ce)
.log("Failed to initialize metadata client driver using invalid metadata service uri");
+ close();
Review Comment:
Good point — done.use Try-finally to handle the close.most of the diff is pure whitespace.please take a look.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4797:Prevent thread leak when BookKeeper client constructor fails [bookkeeper]
Posted by "kroosxu (via GitHub)" <gi...@apache.org>.
kroosxu commented on PR #4798:
URL: https://github.com/apache/bookkeeper/pull/4798#issuecomment-5053968539
Hi @lhotari @merlimat @hangc0276 @StevenLuMT , can you please take a look?
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4797:Prevent thread leak when BookKeeper client constructor fails [bookkeeper]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #4798:
URL: https://github.com/apache/bookkeeper/pull/4798#discussion_r3679589827
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -459,9 +459,11 @@ public BookKeeper(ClientConfiguration conf, ZooKeeper zk, EventLoopGroup eventLo
log.error()
.exception(ce)
.log("Failed to initialize metadata client driver using invalid metadata service uri");
+ close();
throw new IOException("Failed to initialize metadata client driver", ce);
Review Comment:
Calling close() inside this catch can throw InterruptedException (and the method signature also allows BKException), which would mask the original ConfigurationException and change what build()/ctor reports to callers. It’s better to preserve the original failure and treat close() failures as suppressed while restoring the interrupt flag.
This issue also appears on line 464 of the same file.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -537,6 +538,7 @@ public BookKeeper(ClientConfiguration conf, ZooKeeper zk, EventLoopGroup eventLo
this.ledgerManagerFactory =
this.metadataDriver.getLedgerManagerFactory();
} catch (MetadataException e) {
+ close();
throw new IOException("Failed to initialize ledger manager factory", e);
}
Review Comment:
This adds cleanup for getLedgerManagerFactory() failures, but other constructor failure points can still leak previously-created resources because they aren’t wrapped (e.g. initializeEnsemblePlacementPolicy(...) is declared throws IOException at BookKeeper.java:579-592, and BookieClientImpl’s ctor throws IOException). Consider wrapping the whole constructor body in a try/finally with an "initialized" flag and invoking close() in finally when initialization doesn’t complete, rather than sprinkling close() calls in a few catches.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Issue 4797:Prevent thread leak when BookKeeper client constructor fails [bookkeeper]
Posted by "zymap (via GitHub)" <gi...@apache.org>.
zymap commented on code in PR #4798:
URL: https://github.com/apache/bookkeeper/pull/4798#discussion_r3679599789
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeper.java:
##########
@@ -459,9 +459,11 @@ public BookKeeper(ClientConfiguration conf, ZooKeeper zk, EventLoopGroup eventLo
log.error()
.exception(ce)
.log("Failed to initialize metadata client driver using invalid metadata service uri");
+ close();
Review Comment:
I think it's better not to close it internally. It may throw an exception that only happens in the close() method and misdirect the failure case. Try-finally should be a better way to handle the close.
--
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: commits-unsubscribe@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org