Only touch currentForklessSave on the main thread - #4467
Conversation
currentForklessSave was cleared in forklessSaveProcessor, which runs on the worker thread, while forklessSaveCancel reads it (and its iterator) on the main thread. Two threads writing/reading the same pointer with no lock is a data race that can crash the cancel path. Remove the worker-thread clear. The pointer is already cleared on the main thread on every end path: cleanupSaveInfoAndEmitEndMetrics() for a save that started, and the werr path in forklessSaveToDisk() if it failed to start. Now only the main thread accesses currentForklessSave, so cancel can no longer race the clear. Signed-off-by: Nitai Caro <caronita@amazon.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
currentForklessSave is the handle forklessSaveCancel() uses to terminate the running iterator. forklessSaveComplete() set the iterator to NULL but left currentForklessSave non-NULL until the later asynchronous file-close cleanup. During that window a BGSAVE CANCEL would call forklessSaveCancel() and pass the now-NULL iterator to bgIteratorTerminate(), which dereferences it. Clear currentForklessSave together with the iterator so the cancel handle never outlives what it cancels. It stays main-thread-only. Signed-off-by: Nitai Caro <caronita@amazon.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forkless #4467 +/- ##
============================================
- Coverage 79.69% 79.29% -0.41%
============================================
Files 175 175
Lines 94841 93222 -1619
============================================
- Hits 75581 73916 -1665
- Misses 19260 19306 +46
🚀 New features to boost your workflow:
|
2 participants
currentForklessSavewas cleared inforklessSaveProcessor, which runs on the worker thread, whileforklessSaveCancelreads it (and its iterator) on the main thread. Two threads writing/reading the same pointer with no lock is a data race that can crash the cancel path.The contract is that we always pass an
ITEM_CLOSEwhen we finish, which causes the cleanup function to get called which clears upcurrentForklessSaveanyway, so this should be safe.Update: that cleanup runs after the async file close, but
forklessSaveCompletenulls the iterator earlier - so aBGSAVE CANCELin that window would terminate aNULLiterator and crash. So I also clearcurrentForklessSavewhere we null the iterator inforklessSaveComplete