Skip to content

Conversation

@zahiraam
Copy link
Contributor

@zahiraam zahiraam commented Dec 4, 2024

No description provided.

Comment on lines +504 to 505
std::unique_lock<std::mutex> Lock(Mutex);
WithContext Guard(std::move(CurrentReq->Ctx));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, I don't think this solves the issue by itself. Just prior to exiting the above block, we are assured that CurrentReq has been assigned and thus holds an object. At this point, the lock on Mutex has been released which means it is possible that some other thread executing this same function might acquire the lock on Mutex and reset CurrentReq at line 517/518 below before this code is reached. The possibility still exists then that CurrentReq doesn't hold an object at this point (or holds a partially constructed/destructed object; note that std::optional doesn't promise atomic operations).

I'm not sure what the right fix is for this issue at this point. I think it might be to move *NextReq above into a local Request variable and change the CurrentReq data member to just be a bool that indicates whether a request is being processed. That would avoid any possibility of concurrent modification of the Request object while still preserving the ability to check if a request is being processed in blockUntilIdle().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per private discussion, we determined that there isn't actually a bug here so long as a PreambleThread object is only ever scheduled to run on one thread. That appears to be the case; the only instance of a PreambleThread object is the PreamblePeer member of ASTWorker and ASTWorker::create() only spawns one task to run PreambleThread::run(). I think this PR can be abandoned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tahonermann. I had forgotten about it. Closing it.

@zahiraam zahiraam closed this Dec 10, 2024
@zahiraam zahiraam deleted the RaceConditionFix branch December 10, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants