Skip to content

bug fix: s3fifo cache race condition #21862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 26 commits into
base: main
Choose a base branch
from
Draft

bug fix: s3fifo cache race condition #21862

wants to merge 26 commits into from

Conversation

cpegeric
Copy link
Contributor

@cpegeric cpegeric commented May 16, 2025 •
edited
Loading

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #21838

What this PR does / why we need it:

  • The fix includes adding deleted status to avoid double postEvict/remove from hashtable.
  • Gemini AI also review and didn't detect any race condition
  • run test and benchmark with -race flag.
  • set the dellocated buffer to nil and panic when try to access deallocated buffer
  • defer Bytes.Release() at the end of postEvict() function
  • all post functions should be protected by shardmap mutex to prevent race condition between cache data and shardmap.

@cpegeric cpegeric requested review from reusee and fengttt as code owners May 16, 2025 09:31
@cpegeric cpegeric marked this pull request as draft May 16, 2025 09:31
@mergify mergify bot added the kind/bug Something isn't working label May 16, 2025
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 16, 2025
@matrix-meow matrix-meow added size/XL Denotes a PR that changes [1000, 1999] lines and removed size/M Denotes a PR that changes [100,499] lines labels May 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fengttt fengttt fengttt left review comments

@reusee reusee Awaiting requested review from reusee reusee is a code owner

@LeftHandCold LeftHandCold Awaiting requested review from LeftHandCold LeftHandCold will be requested when the pull request is marked ready for review LeftHandCold is a code owner

@XuPeng-SH XuPeng-SH Awaiting requested review from XuPeng-SH XuPeng-SH will be requested when the pull request is marked ready for review XuPeng-SH is a code owner

@zhangxu19830126 zhangxu19830126 Awaiting requested review from zhangxu19830126 zhangxu19830126 will be requested when the pull request is marked ready for review zhangxu19830126 is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
kind/bug Something isn't working size/XL Denotes a PR that changes [1000, 1999] lines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cpegeric @fengttt @matrix-meow