Close the consumer when removeMessageConsumer removes it - #1847
Merged
Conversation
removeMessageConsumer only called stop(), which asks the polling thread to finish its current loop. The MessageConsumer, its session and its connection were left open, so the broker went on dispatching to a consumer nobody was reading: with prefetchLimit=1 one message sat in it and was not redelivered until the connection died. ConsumerContextJms.stopAndClose() now stops the thread, waits for it to leave its receive() (bounded by two polling intervals, the same budget close() uses), and then releases the JMS resources. This is not hypothetical -- it cost a full debugging cycle on #1845, where a responder torn down by one test still held a prefetched message and made a later test look like a concurrency bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SY1XHgTZXZPqUECo2VBAWg
The push that opened #1847 landed during a GitHub Actions major outage, so no workflow run was ever created for it -- the PR sat with zero checks and nothing retroactively schedules one. ci.yml fires on push, not on pull_request, so an empty commit is what re-triggers it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SY1XHgTZXZPqUECo2VBAWg
stopAndClose() closes the MessageConsumer while the polling thread may still be
parked in receive(). Two problems made that catastrophic rather than merely
untidy:
- bProcessing was not volatile, so the polling thread could miss stop()
entirely and keep looping;
- receive() on a closed consumer throws immediately, and the loop logged it
via onException() and went straight round again.
The result was a tight spin logging "The Consumer is closed" as fast as the CPU
allows: 2,125,490 such lines in one local run of MessageProducerSessionJmsTest.
In CI it starved the shard badly enough that rpcRoundTripReturnsTheAnswer timed
out after 30s, which is how it surfaced.
bProcessing and thread are now volatile, and a consumer closed while polling is
treated as shutdown -- logged at debug and the loop exits -- rather than as an
error to report and retry. Same run now produces 0 such lines.
This could not happen before the previous commit, because removeMessageConsumer
never closed the consumer at all; it just leaked it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01SY1XHgTZXZPqUECo2VBAWg
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
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.
Last of the JMS resource problems turned up while working through the export-progress
incident (#1837, #1838, #1839, #1842, #1844, #1845).
The leak
removeMessageConsumeronly calledstop(), which asks the polling thread to finish itscurrent loop:
The
MessageConsumer, its session and its connection were left open. The broker thereforewent on dispatching to a consumer nobody was reading — and with a prefetch limit those
messages sit in its client-side buffer, undelivered and not redelivered until the
connection dies. Every VCell consumer is registered with an explicit prefetch limit
(
MessageConstants.PREFETCH_LIMIT_*), so any removed consumer strands at least one message.The fix
ConsumerContextJms.stopAndClose()stops the thread, waits for it to leave itsreceive()— bounded by two polling intervals, the same budget
VCMessagingServiceJms.close()alreadyallows — and then closes the consumer, session and connection.
removeMessageConsumerusesit.
Finding the context first and mutating the list afterwards also removes a
remove-while-iterating that only worked because of the immediate
return.Verification
removingAConsumerDoesNotStrandMessagesregisters a consumer, removes it, sends threemessages, then registers a replacement and requires all three to arrive. Control, with both
production files reverted to
masterand the test kept:Exactly one message stranded, as predicted by the prefetch limit of 1.
vcell-serverFastgroup: 61 passed.
This one was not hypothetical
It cost a full debugging cycle on #1845. A responder torn down by an earlier test in the
same class was still holding a prefetched message, so a later test failed in a way that
looked exactly like the concurrency bug that PR was fixing — four concurrent callers passed,
six did not. Running the same test against
origin/masterfirst is what ruled out thechange under test and pointed here instead.
In production the same shape means a request silently stranded whenever a consumer is
removed and re-registered: no error, no redelivery, just one message that never gets a
reply until the connection is torn down.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SY1XHgTZXZPqUECo2VBAWg