fix: stop the kafka watch thread when the client is closed - #170
Open
TaurMorchant wants to merge 3 commits into
Open
fix: stop the kafka watch thread when the client is closed#170TaurMorchant wants to merge 3 commits into
TaurMorchant wants to merge 3 commits into
Conversation
KafkaMaaSClientImpl.close() interrupted the watchTopicCreate thread, but the thread only left its loop on an InterruptedException around wait(). While topic listeners were still registered, it sat in the inner polling loop, which swallows every exception and retries with no delay, so the thread outlived the client and kept hammering the agent. In tests this leaked thread flooded the MockServer shared by KafkaMaaSClientImplTest until it stopped responding, which failed unrelated tests in the class and blew prepare.log up to 569 MB during the monorepo release. The interrupt flag alone cannot drive the exit: the retry branch in HttpExecution calls Thread.sleep(), which clears the flag before the watch loop sees it. Track shutdown in a dedicated volatile field instead, and check it before the thread parks in wait(). KafkaMaaSClientCloseTest pins the invariant: once close() returns, the watch thread is gone. It uses its own lightweight HTTP stub rather than MockServer, so a regression cannot spill over into neighboring tests.
JUnit builds a fresh instance per test method, so resetting the latches, the flag and the thread reference in the setup method was dead code. Initialize the fields inline instead. Drop the cached thread pool: the stub serves one client, and a user-supplied executor is not shut down by HttpServer.stop(), so it leaked threads on every run.
github-actions
Bot
added
the
documentation
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
KafkaMaaSClientImpl.close()did not stop thewatchTopicCreatethread while topic listeners were still registered.The leaked thread kept polling the agent with no delay between attempts, which flooded the MockServer shared by
KafkaMaaSClientImplTestuntil it stopped answering. That failed unrelated tests in the class and blewmaas-client/prepare.logup to 569 MB, breaking two consecutivelts/26.3release runs(31105532757,
31109752591).
Details and log evidence: #169.
What
KafkaMaaSClientImpltracks shutdown in avolatile boolean closed, checks it in the watch loop, and returnsbefore the thread parks in
wait().close()sets the flag; the existinginterrupt()stays, since it is what aborts an in-flight 60 s long poll.KafkaMaaSClientCloseTestpins the invariant: onceclose()returns, the watch thread is gone.The interrupt flag alone cannot drive the exit. The retry branch in
HttpExecution.sendAndReceive()callsThread.sleep(), andThread.sleep()clears the flag when it throws, so the watch loop never sees it. The client logfrom a failing run shows the thread carrying on right after the interrupt:
The test deliberately does not use MockServer. It runs its own
com.sun.net.httpserver.HttpServerstub that answersthe watch endpoint slowly and successfully, so a regression shows up as a dead-or-alive assertion on the thread rather
than as collateral damage to whichever test happens to run next.
How to verify
mvn -B -ntp -f maas-client/client/pom.xml testResult on this branch:
KafkaMaaSClientCloseTestwas run three times againstmainbefore the fix and failed all three times with the sameassertion, then three times after the fix and passed all three.
Across the whole module suite the log now contains 42
[watchTopicCreate]lines. The failing release run producedroughly 7.6 million.