overlay v2: fix ipc concurrency race - #5425
Conversation
9e25ddd to
24284b1
Compare
SirTyson
left a comment
SirTyson
left a comment
There was a problem hiding this comment.
I'm not super familiar with this codebase yet, so I have a couple questions. It seems incorrect to hold the lock for both "sides" of an operation here, but I could be misunderstanding the threading pattern.
| // unblock us. The wait is bounded so a lost response degrades to an | ||
| // empty candidate list instead of wedging the main thread. | ||
| bool gotResponse = | ||
| mRequestCv.wait_for(lock, std::chrono::seconds(5), [this] { |
There was a problem hiding this comment.
Is it still safe to hardcode this as 5 seconds if we're testing much smaller block times?
There was a problem hiding this comment.
this doesn't really have anything to do with block time, it just lets core fail gracefully in case overlay gets stuck (which can happen in this prototype phase). I don't feel too strongly about this timeout, but it does help supercluster fail properly (instead of spinning for hours)
| // reset afterwards would discard it and we'd wait forever. Lock order | ||
| // (mRequestMutex -> mSendMutex) is safe: the reader's handleMessage only | ||
| // takes mRequestMutex, and no other path nests these two. | ||
| std::unique_lock<std::mutex> lock(mRequestMutex); |
There was a problem hiding this comment.
I don't think we need to actually need these nesting locks, do we? If I understand correctly, the original race was receiving the payload we requested before the reset. Is changing the order of the operations not enough, such that we can do
{
lock(request);
reset();
}
{
lock(send);
send();
}
{
lock(request);
cv.wait(lock);
}
It seems like the real race was reseting in a window in which we may have already received our payload. Just swapping the calling order achieves this by itself right? It seems like a bad idea to hold both a request/send mutex simultaneously, where we could get some weird stall patterns under queue load.
There was a problem hiding this comment.
yeah the race was related to ordering, i agree the mutex nesting wasn't necessary
getTopTransactions sent the GET_TOP_TXS request before locking mRequestMutex and resetting mPendingResponse. If the reader thread delivered TOP_TXS_RESPONSE in that window (round-trip is ~0.4ms, so a descheduled main thread under load hits this), the reset discarded the response and the wait -- which had no timeout -- blocked the main thread forever. In simulation tests this wedges every node in the process; observed as a permanently stuck 15-node 2000 TPS stress test. Fix: reset the response slot before sending, holding mRequestMutex across the send (lock order mRequestMutex -> mSendMutex is acyclic: the reader only takes mRequestMutex), and bound the wait at 5s so a lost response degrades to an empty candidate list instead of a hung validator. requestMetrics had the identical reset-after-send race, previously masked by its timeout as spurious metric-timeout warnings; apply the same reset-before-send fix there. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
24284b1 to
282fa63
Compare
2 participants
No description provided.