Fix[mqb]: Re-read counter after de-configure response - #1316
Conversation
dorjesinpo
changed the title
|
|
| const bmqp_ctrlmsg::StreamParameters& streamParameters, | ||
| mqbi::Queue* queue, | ||
| mqbi::QueueHandle* handle, | ||
|
|
| mqbi::QueueHandle::HandleReleasedCallback()); | ||
| if (info.d_counts.d_readCount == 0) { | ||
| // Already fully released by cookie rollback — nothing to do. | ||
| } |
There was a problem hiding this comment.
I think this is missing a return;. Else we still release the handle.
|
|
||
| const bool isFinal = (counter->decrement() == 0); | ||
| // Re-read current counts from the handle because cookie rollback may have | ||
| // already decremented some counts between de-confgigure request and now. |
d7b5d99 to
d2c5916
Compare
|
This PR is incomplete. We need to handle the other racer - |
d27353a to
39fafed
Compare
|
Taking a look... |
| d_queueHandleRequesterContext_sp.createInplace(d_allocator_p, | ||
| d_allocator_p); | ||
|
|
||
| d_queueHandleRequesterContext_sp->setClient(this) |
There was a problem hiding this comment.
Comment, no action needed: This fluent builder pattern here didn't fully pass the sniff test to me, because the default object constructed is not valid (hence, fluent pattern is an anti-pattern). This made me look into QueueHandleRequesterContext a little more, and I found this lovely contradiction:
/// Value-semantic type representing the context of a client, requester of a
/// queue handle.
class QueueHandleRequesterContext {
and
/// Copy constructor and assignment operator are not implemented.
QueueHandleRequesterContext&
operator=(const QueueHandleRequesterContext&) BSLS_CPP11_DELETED;
😮💨
From inspection of where it's used, I think we can very easily remove the invalid state from QueueHandleRequesterContext and make this type safe and impossible to misuse.
Let's get this change in as it is for the functionality change, and I'll prepare a separate PR that simplifies this.
| /// the request was success and the specified `queueHandle` contains the | ||
| /// handle representing the queue that was allocated for this specified | ||
| /// `requester` session having the specified `peerInstanceId`, and the | ||
| /// `requester` session having the specified `requesterId`, and the |
There was a problem hiding this comment.
This rename wasn't done completely:
src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.cpp
2582: const int peerInstanceId)
2594: peerInstanceId),
2603: const int peerInstanceId)
2683: peerInstanceId));
2743: << ", initial peerInstanceId: " << requesterId
2744: << ", current peerInstanceId: "
src/groups/mqb/mqbblp/mqbblp_clusterqueuehelper.h
697: int peerInstanceId);
701: /// specified `requester` with the specified `peerInstanceId`. If the
710: const int peerInstanceId);
I think the log message particularly is confusing.
| return; // RETURN | ||
| } | ||
|
|
||
| // TODO: revisit |
There was a problem hiding this comment.
Can we capture this in an issue with some context? We have almost 200 TODOs in the codebase; this will get lost.
There was a problem hiding this comment.
Created a card "Revisit logic around NodeStatus, ElectorInfoLeaderStatus, PrimaryStatus"
| .setDescription(bsl::string(description(), d_allocator_p)) | ||
| .setIsClusterMember(true) | ||
| .setRequesterId( | ||
| mqbi::QueueHandleRequesterContext ::generateUniqueRequesterId()) |
There was a problem hiding this comment.
mqbi::QueueHandleRequesterContext::generateUniqueRequesterId()
|
|
||
| // PRECONDITIONS | ||
| BSLS_ASSERT_SAFE(inDispatcherThread()); | ||
| d_queueHandleRequesterContext_sp.createInplace(d_allocator_p, |
There was a problem hiding this comment.
This is a little subtle. createQueueHandleRequesterContext pulls out the ClientIdentity and StatContext out of the old queue handle requester context, passes them into createQueueHandleRequesterContext by const&, and then calls createInplace. Are the objects these references point to still valid? Depends on whether there's any other shared_ptr to the old handle requester context, and if not, whether createInplace calls the destructor of the existing object before it calls the constructor of the new object. This isn't guaranteed, but either way it's very subtle.
I think it's better to avoid that subtle reasoning, since it's fragile next time we change this. Let's add some copies before we createInplace:
// Copy before `createInplace` destroys the old object.
const bmqp_ctrlmsg::ClientIdentity identity = d_queueHandleRequesterContext_sp->identity();
const bsl::shared_ptr<bmqst::StatContext> statContext = d_queueHandleRequesterContext_sp->statContext();
There was a problem hiding this comment.
On further thought, we can put this copy in createQueueHandleRequesterContext, since the ...Impl function is called in the constructor too.
| bmqu::GateKeeper& gatePut(); | ||
| bmqu::GateKeeper& gateConfirm(); | ||
|
|
||
| void createQueueHandleRequesterContext(); |
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
39fafed to
9314743
Compare
Signed-off-by: dorjesinpo <129227380+dorjesinpo@users.noreply.github.com>
2 participants
Concurrent cookie rollback (
onOpenQueueConfirmationCookieReleased) may decrement the same count asdropHandleDispatchedresulting in double decrement.Solution: re-read current counts from handle->subStreamInfos() upon de-configure response.