Skip to content
New issue

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

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

Already on GitHub? # to your account

[fix][txn] Fix SnapshotSegmentAbortedTxnProcessorImpl thread safety issue #22384

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

nikam14
Copy link
Contributor

@nikam14 nikam14 commented Mar 29, 2024

Fixes #22116

Motivation

Fix SnapshotSegmentAbortedTxnProcessorImpl thread safety issue

Modifications

  1. Added volatile keyword to unsealedtxnIds variable.
  2. Same resource were accessed at same line , so modified it.
  3. Added synchronized keyword for data consistency

Verifying this change

  • Make sure that the change passes the CI checks.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository:

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 29, 2024
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @nikam14 .

Unfortunately fixing these issues isn't simple. This PR might be doing the right things, but without proper concurrency tests, it's hard to ensure that adding synchronized methods doesn't cause regressions such as dead-locks. The lack of tests isn't caused by this PR, but to get confidence for merging such PRs, it would have to be resolved.
Just viewing the code won't be a sufficient review.

@nikam14
Copy link
Contributor Author

nikam14 commented Apr 5, 2024

Thanks for the explanation @lhotari

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
doc-not-needed Your PR changes do not impact docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] SnapshotSegmentAbortedTxnProcessorImpl contains thread safety issues
2 participants