Withhold link credit until the connection is started - #76
Open
ansd wants to merge 1 commit into
Open
Conversation
A consumer created on a connection that has not been started was granted its full prefetch of link credit straight away, in the flow frame that follows its attach. The remote is then free to send, and it does: the messages are transferred on that consumer's link and buffered client side, where the application cannot see them until Connection.start() is called. Jakarta Messaging 6.1.4 says a connection is created in stopped mode, "that means that no messages are being delivered to it", and is emphatic about the guarantee: "clients rely on the fact that no messages will be delivered to a consumer until its connection has been started. Jakarta Messaging Providers must ensure that this is the case." Granting credit before start defeats that. The messages have been delivered to the consumer in every sense the protocol recognises - transferred on its link, and held as unsettled deliveries owned by it at the peer. Only the last hop, into the application, is withheld. That is not merely a semantic quibble, because delivery is exclusive. Once a message has been transferred to one consumer's link it is no longer available to any other consumer on that queue. A consumer on a connection that is never started therefore takes messages that a started consumer on the same queue can never receive. If that connection stays open, the messages are stranded for its lifetime, invisible to every application. This is what the Jakarta Messaging TCK trips over in core20/jmsconsumertests. JmsTool's queue setup creates a connection, a session, a producer and a consumer, and never starts that connection; the test then opens a JMSContext of its own, whose consumer is started, sends one message and blocks in receive(). Both links hold credit, so the peer picks one, and the outcome depends on which - a detail no specification defines. Against RabbitMQ, which selects in attach order, the never-started consumer attached first and always won, so the started consumer blocked forever and three tests hung until killed. Swapping the attach order alone makes the same test pass, which shows how thin the ice is: brokers that select differently pass today with this bug present and unfixed. So grant credit at the point where the specification allows delivery to begin. JmsMessageConsumer.init() now only starts the consumer resource if the session is already started, and JmsSession.start() starts the resource for the consumers that were created while it was stopped. Both paths are needed: JmsSession.start() is once-only and iterates the consumers that exist at the time, so a consumer created after the connection was started - which 6.1.4 explicitly permits - would otherwise never be credited. Between them each consumer is credited exactly once, at the right moment. The pairing of JmsMessageConsumer.start() with startConsumerResource() is the one resumeAfterRollback() already uses for the same purpose. Two existing tests asserted the previous behaviour directly, expecting credit and inbound transfers on a connection that was never started, and checking only that receive() returned null. What they are really about is that the application does not see messages while delivery is paused, which is equally true of a connection that has been stopped, so they now start the connection, let the messages arrive, and stop it before asserting. They are renamed to testNoReceivedMessagesWhenConnectionStopped and testNoReceivedNoWaitMessagesWhenConnectionStopped. Twenty other tests created a consumer on an unstarted connection incidentally, for reasons that have nothing to do with delivery, and simply start the connection now. Connection.stop() has the same shape of problem and is not addressed here: it stops the client side message queue but leaves the link credit in place, so a stopped connection's consumer also goes on taking messages from its queue. Fixing that means draining credit and waiting out the in-flight deliveries, as suspendForRollback() does, and is left for separate work.
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.
A consumer created on a connection that has not been started was granted its full prefetch of link credit straight away, in the flow frame that follows its attach. The remote is then free to send, and it does: the messages are transferred on that consumer's link and buffered client side, where the application cannot see them until Connection.start() is called.
Jakarta Messaging 6.1.4 says a connection is created in stopped mode, "that means that no messages are being delivered to it", and is emphatic about the guarantee: "clients rely on the fact that no messages will be delivered to a consumer until its connection has been started. Jakarta Messaging Providers must ensure that this is the case." Granting credit before start defeats that. The messages have been delivered to the consumer in every sense the protocol recognises - transferred on its link, and held as unsettled deliveries owned by it at the peer. Only the last hop, into the application, is withheld.
That is not merely a semantic quibble, because delivery is exclusive. Once a message has been transferred to one consumer's link it is no longer available to any other consumer on that queue. A consumer on a connection that is never started therefore takes messages that a started consumer on the same queue can never receive. If that connection stays open, the messages are stranded for its lifetime, invisible to every application.
This is what the Jakarta Messaging TCK trips over in core20/jmsconsumertests. JmsTool's queue setup creates a connection, a session, a producer and a consumer, and never starts that connection; the test then opens a JMSContext of its own, whose consumer is started, sends one message and blocks in receive(). Both links hold credit, so the peer picks one, and the outcome depends on which - a detail no specification defines. Against RabbitMQ, which selects in attach order, the never-started consumer attached first and always won, so the started consumer blocked forever and three tests hung until killed. Swapping the attach order alone makes the same test pass, which shows how thin the ice is: brokers that select differently pass today with this bug present and unfixed.
So grant credit at the point where the specification allows delivery to begin. JmsMessageConsumer.init() now only starts the consumer resource if the session is already started, and JmsSession.start() starts the resource for the consumers that were created while it was stopped. Both paths are needed: JmsSession.start() is once-only and iterates the consumers that exist at the time, so a consumer created after the connection was started - which 6.1.4 explicitly permits - would otherwise never be credited. Between them each consumer is credited exactly once, at the right moment. The pairing of JmsMessageConsumer.start() with startConsumerResource() is the one resumeAfterRollback() already uses for the same purpose.
Two existing tests asserted the previous behaviour directly, expecting credit and inbound transfers on a connection that was never started, and checking only that receive() returned null. What they are really about is that the application does not see messages while delivery is paused, which is equally true of a connection that has been stopped, so they now start the connection, let the messages arrive, and stop it before asserting. They are renamed to testNoReceivedMessagesWhenConnectionStopped and
testNoReceivedNoWaitMessagesWhenConnectionStopped. Twenty other tests created a consumer on an unstarted connection incidentally, for reasons that have nothing to do with delivery, and simply start the connection now.
Connection.stop() has the same shape of problem and is not addressed here: it stops the client side message queue but leaves the link credit in place, so a stopped connection's consumer also goes on taking messages from its queue. Fixing that means draining credit and waiting out the in-flight deliveries, as suspendForRollback() does, and is left for separate work.