Hiroshi Ikeda Thanks for taking a look.
Although ReadWriteLock is much more complex than simple locks and is not prefered to enclose trival logic, I suspect it is not severe overhead compared to LinkedBlockingQueue.
One of the issues with RWLock I found was that we'd have to use offer(timeout) instead of put(), or else the readlock may not be released. This makes the queue swap lock for about a second, and causes a huge backup in the process. Otherwise the overhead of the lock wasn't much, it's mainly the offer(timeout).
By the way, about the patch, nobody can eliminate the possibility that a blocked thread at LinkedBlockingQueue.put() cannot wake up in 1 seconds when another thread drains. At least you should check the reference after put(), such as
We could increase the timeout period on the queue swap transfer too.
(I also worry about the order of elements is not preserved in spite of the name "queue".)
Order doesn't matter to the client, since the client cannot expect to send two consecutive commands without receiving a response from the first, and expect order that they're executed in order, even today. This is why we can even do QoS by re-ordering rpc calls in the first place.
As far as the server goes, I guess this will have to be something we're aware of.
Still, implementation of size() is invalid.
Are you referring to CallQueueManager.size(), due to the possibility that the queue is being swapped? I'm assuming that size() isn't being used for anything besides metrics, so a small discrepancy during swapping would be okay.
The atomic reference is not needed for the queue and volatile is enough if you only set and get. Volatile variables may have much chance to be optimized by VM because volatile is within the language specification. On the other hand, increment operation (++) is not atomic for volatile variables, so some of the test classes should be changed.
Agreed, though I think Daryn Sharp says he prefers atomicref for clarity. I could go either way.
Also, in the tests, the atomic members aren't being written by multiple threads–they're atomic so that I can read them from another thread without thread caching. Even if they incremented atomically, there's still the issue of the put happening before the increment, so the tests only check after the operations complete (using Thread.sleeps)