[
https://issues.apache.org/jira/browse/ZOOKEEPER-1863?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Dutch T. Meyer updated ZOOKEEPER-1863:
--------------------------------------
Attachment: ZOOKEEPER-1863.patch
Here is a sketch at one approach. I'd appreciate feedback on this - I don't
consider it particularly elegant, and I hope there's a better way.
The idea is straightforward - we check queuedRequests prior to the dequeue of
committedRequests to ensure that the head of commitedRequests has not raced.
Since I'd rather not take a full traversal for every sync request, I've further
optimized this by wrapping the whole block in a check on isWaitingForCommit.
If nextPending is not NULL I don't believe the syncs can jump ordering. So we
should only pay the cost of checking if we receive a commit we weren't already
waiting on, and if I'm not mistake that requires that the block above exited
with:
{noformat}
queuedRequests.poll() == null
{noformat}
So the queue probably hasn't grown so deep in the interim that the traversal is
particularly expensive.
Still - The dependencies between the blocks of in this loop are pretty subtle
and hard to understand. If someone can safely refactor it I think that would
be much preferred. It might also be better to tag commit/sync requests such
that this check for identity:
{noformat}
pending.sessionId == request.sessionId &&
pending.cxid == request.cxid
{noformat}
is a bit stronger. If we knew in this commit processor where the request came
from (i.e. was it processed by our parent FollowerRequestProcessor?) then the
above test would be cleaner and this race would be easy to avoid.
> Race condition in commit processor leading to out of order request
> completion, xid mismatch on client.
> ------------------------------------------------------------------------------------------------------
>
> Key: ZOOKEEPER-1863
> URL: https://issues.apache.org/jira/browse/ZOOKEEPER-1863
> Project: ZooKeeper
> Issue Type: Bug
> Components: server
> Affects Versions: 3.5.0
> Reporter: Dutch T. Meyer
> Priority: Blocker
> Attachments: ZOOKEEPER-1863.patch, stack.17512
>
>
> In CommitProcessor.java processor, if we are at the primary request handler
> on line 167:
> {noformat}
> while (!stopped && !isWaitingForCommit() &&
> !isProcessingCommit() &&
> (request = queuedRequests.poll()) != null) {
> if (needCommit(request)) {
> nextPending.set(request);
> } else {
> sendToNextProcessor(request);
> }
> }
> {noformat}
> A request can be handled in this block and be quickly processed and completed
> on another thread. If queuedRequests is empty, we then exit the block. Next,
> before this thread makes any more progress, we can get 2 more requests, one
> get_children(say), and a sync placed on queuedRequests for the processor.
> Then, if we are very unlucky, the sync request can complete and this object's
> commit() routine is called (from FollowerZookeeperServer), which places the
> sync request on the previously empty committedRequests queue. At that point,
> this thread continues.
> We reach line 182, which is a check on sync requests.
> {noformat}
> if (!stopped && !isProcessingRequest() &&
> (request = committedRequests.poll()) != null) {
> {noformat}
> Here we are not processing any requests, because the original request has
> completed. We haven't dequeued either the read or the sync request in this
> processor. Next, the poll above will pull the sync request off the queue, and
> in the following block, the sync will get forwarded to the next processor.
> This is a problem because the read request hasn't been forwarded yet, so
> requests are now out of order.
> I've been able to reproduce this bug reliably by injecting a
> Thread.sleep(5000) between the two blocks above to make the race condition
> far more likely, then in a client program.
> {noformat}
> zoo_aget_children(zh, "/", 0, getchildren_cb, NULL);
> //Wait long enough for queuedRequests to drain
> sleep(1);
> zoo_aget_children(zh, "/", 0, getchildren_cb, &th_ctx[0]);
> zoo_async(zh, "/", sync_cb, &th_ctx[0]);
> {noformat}
> When this bug is triggered, 3 things can happen:
> 1) Clients will see requests complete out of order and fail on xid mismatches.
> 2) Kazoo in particular doesn't handle this runtime exception well, and can
> orphan outstanding requests.
> 3) I've seen zookeeper servers deadlock, likely because the commit cannot be
> completed, which can wedge the commit processor.
--
This message was sent by Atlassian JIRA
(v6.1.5#6160)
