fix(bufferedwrites): prevent memory leak & race in UploadHandler.Destroy - #5026
fix(bufferedwrites): prevent memory leak & race in UploadHandler.Destroy#5026meet2mky wants to merge 1 commit into
Conversation
90601ec to
66b3bac
Compare
There was a problem hiding this comment.
Code Review
This pull request updates the Destroy method in UploadHandler to cancel ongoing GCS uploads and wait for the uploader goroutine to finish processing in-flight blocks. It also adds a unit test to verify this behavior. The review feedback points out that using time.Sleep in the new unit test to synchronize goroutines is an anti-pattern that can cause flakiness, and suggests using a channel-based synchronization approach instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5026 +/- ##
==========================================
+ Coverage 83.83% 83.91% +0.08%
==========================================
Files 174 174
Lines 21378 21441 +63
==========================================
+ Hits 17923 17993 +70
+ Misses 2776 2758 -18
- Partials 679 690 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
66b3bac to
73f4253
Compare
…ndler.Destroy When a file handle is destroyed (e.g., after a failed upload or unlink), UploadHandler.Destroy did not cancel the ongoing GCS upload context or wait for the background uploader goroutine to finish. This caused the uploader goroutine to return its in-flight block to the block pool *after* ClearFreeBlockChannel had already executed. In production, this resulted in: 1. Memory Leaks: The late-arriving block was never munmap'ed. 2. Write Stalls: The global semaphore permit (globalMaxBlocksSem) for the block was never released, eventually exhausting the semaphore and causing all new writes on the mount to hang indefinitely. 3. Wasted Bandwidth: GCS uploads continued running in the background for closed/aborted files. This commit fixes the teardown sequence: - Cancels the GCS upload context inside Destroy() to quickly terminate the in-flight upload. - Adds uh.wg.Wait() to ensure the uploader goroutine completes and releases its block before the block pool is cleared. - Introduces TestDestroyWithUploaderGoroutineInProgress to assert correct synchronization.
73f4253 to
976c7f0
Compare
|
Hi @vadlakondaswetha, @vipnydav, @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
1 similar comment
|
Hi @vadlakondaswetha, @vipnydav, @abhishek10004, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you! |
Description
When a file handle is destroyed (e.g., after a failed upload or unlink),
UploadHandler.Destroydid not cancel the ongoing GCS upload context or wait for the backgrounduploadergoroutine to finish processing its current chunk.This caused the
uploadergoroutine to return its in-flight block back to the block pool afterClearFreeBlockChannelhad already executed and finished teardown. In production, this resulted in severe consequences:Deallocate()was never called and the underlyingmmapmemory was never freed.globalMaxBlocksSem) tied to the abandoned block was never released. Over time, these leaked permits would exhaust the semaphore capacity, causing all new writes across the GCSFuse mount to use staged writes.This PR fixes the teardown sequence and synchronization:
Destroy()to quickly abort the in-flight upload.uh.wg.Wait()after draininguploadChto ensure theuploadergoroutine fully completes and releases its block before teardown continues.TestDestroyWithUploaderGoroutineInProgressto assert strict synchronization betweenDestroyand theuploader.How Has This Been Tested?
go test -v -count=100 -run TestBufferedWriteTestSuite/TestDestroyShouldClearFreeBlockChannel ./internal/bufferedwrites/...and verified it passes 100% of the time.TestDestroyWithUploaderGoroutineInProgressunit test to directly trigger and assert this race condition.Link to the issue in case of a bug fix.
b/546299575
Testing details
Any backward incompatible change? If so, please explain.
NA