Avoid Keeper crash on shutdown (fix test_keeper_snapshot_on_exit)#44908
Avoid Keeper crash on shutdown (fix test_keeper_snapshot_on_exit)#44908antonio2368 merged 1 commit intomasterfrom
test_keeper_snapshot_on_exit)#44908Conversation
azat
left a comment
There was a problem hiding this comment.
@antonio2368 thank you for the fix!
| true /*is_local*/); | ||
| for (const auto & response : responses) | ||
| if (!responses_queue.push(response)) | ||
| if (!responses_queue.push(response) && !responses_queue.isFinished()) |
There was a problem hiding this comment.
Maybe it is better to move this check out from loop?
There was a problem hiding this comment.
The question: is it possible that the queue is finished between the loop iterations? If so it's better to add:
if (responses_queue.isFinished())
break;There was a problem hiding this comment.
To address this and the comment above, the biggest problem is the NuRaft.
If I throw an exception from one of its threads, std::abort happens (that's what caused the problem), that lib doesn't handle exceptions nicely as CH. Assume all methods here are called by NuRaft and not CH.
The queue can be finished in between the loop iterations but it can also be finished between the checks:
if finished -> finished is false
break;
if (!push(...)) -> finished is true, i.e we have same problem
I can add the check for break to not do unnecessary iterations as @novikd suggested, but because I would still need to keep the check after an unsuccessful push I wouldn't change much code-wise.
So I would like to merge this to fix CI ASAP and then polish it.
| LOG_DEBUG(log, "Session ID response {} with timeout {}", session_id, session_id_request.session_timeout_ms); | ||
| response->session_id = session_id; | ||
| if (!responses_queue.push(response_for_session)) | ||
| if (!responses_queue.push(response_for_session) && !responses_queue.isFinished()) |
There was a problem hiding this comment.
Maybe it is better to move this check to the beginning of the function?
Also what will be if exception will be thrown if queue is finished? Or at least some log message maybe?
@antonio2368 what do you think?
| true /*is_local*/); | ||
| for (const auto & response : responses) | ||
| if (!responses_queue.push(response)) | ||
| if (!responses_queue.push(response) && !responses_queue.isFinished()) |
There was a problem hiding this comment.
The question: is it possible that the queue is finished between the loop iterations? If so it's better to add:
if (responses_queue.isFinished())
break;
Changelog category (leave one):
Fix the issue found by
test_keeper_snapshot_on_exitafter #44881 was merged.