hornetq/hornetq — pull request #1098 by jdricklefs (2013-05-29 18:00:47+00:00)
Title: HORNETQ-663 (Rework dead subscriptions reaping in REST interface)

This fixes an issue where the dead subscription culler in the REST interface will block creation of new subscriptions while it runs. I am assuming that is what HORNETQ-663 was referencing, but I may be wrong.

The problem is essentially...
- The TimeoutTask thread synchronizes on itself while it tests subscriptions for deadness.
- The SubscriptionsResource which executes the testTimeout() method will synchronize on each QueueConsumer as it runs.
- If a QueueConsumer is currently polling with an Accept-Wait: value, then the testTimeout() method will have to wait until the polling times out to continue (or a message is recieved, which interrupts the poll).
- Thus, given a long-enough Accept-Wait, or enough accept-waiting-consumers, new pull-subscriptions may internally time out on creation.

This was manifesting itself as a 500 or 404 error (404 if the 500 error page could not be found) in our application (running on JBoss AS 7.1).

This patch changes a few things:
- TimeoutTask now uses an explicit ReentrantLock to manage access to the primary callbacks Map, rather than synchronizing on itself.
- There is a secondary Map which will be used if a new Callback tries to add itself while the first lock is busy, preventing timeouts.
- The Callback interface differentiates between testTimeout and shutdown (previously, if the timeout failed, then it immediately shutdown the QueueConsumer.)
- The testing of timeouts is done to completion before any shutdown() calls, so that the primary Map lock can be released sooner.
- The ping() method on a QueueConsumer now includes an offset (such that if you have an Accept-Wait header, the ping will include that at the start). Otherwise, it would be possible to have an Accept-Wait longer than your timeout value and attempt to shutdown an open socket.

I've included a simple integration test that fails on master (ec5da6a38a) and passes on my branch. That said, it's not great, as it adds a fairly static 11s runtime to the overall test suite.

Let me know if you have any questions!

As a side note, this is my first pull-request to any OSS project, ever. If I've messed up in procedure or best practices, please let me know.