Withhold link credit until the connection is started - #76
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.
|
To begin, we dont consider this a bug as you have described it to be. It is long known behaviour and at the time was quite deliberate, hence the existing tests related behaviour. Prefetch as a concept is something that largely sits outwith what JMS covers, e.g multi-consumer ordering and priority handling are things it interacts with heavily but nothing is said to that. The client has configuration to adjust its prefetch behaviour and applications are expected to set it according to their specific needs if the default isnt to their specific needs/liking. That would apply here just as much, for example you could disable prefetch for your TCK runs by configuring it to 0. Even if still considering this as an improvement, I'll need some time to fully consider the code and behaviour change, but skimming it I see you modified lots of tests to start the connection and say '// Consumers only get link credit once the connection is started.' even though I guess from the names many of those tests never needed or intended to transfer messages. It may be nicer to remove the expectation for a flow in such cases instead. |
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.