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

Jump to bottom

fix: prevent possible concurrent modification of a collection #1370

Merged
merged 1 commit into from
Jan 31, 2025
Merged

fix: prevent possible concurrent modification of a collection #1370

merged 1 commit into from
Jan 31, 2025

Conversation

triceo
Copy link
Contributor

@triceo triceo commented Jan 31, 2025

We have seen reports of the TreeMap getting concurrently accessed and throwing exceptions. Unfortunately, we have never seen any code to reproduce these failures, as concurrency issues are notoriously hard to reproduce. Therefore we refactor the mechanism to make any such issues impossible.

For the same reason as there is no reproducer, there is also no new test coverage. But existing test coverage still passes.

@triceo triceo requested review from rsynek and zepfred January 31, 2025 06:44
@triceo
Copy link
Contributor Author

triceo commented Jan 31, 2025 •
edited
Loading

I have put this together with @rsynek, who is the original author of this code.
I also need a review from @zepfred, who touched this code most recently.
It's concurrency - one can never be too sure.

@triceo triceo added this to the v1.19.0 milestone Jan 31, 2025
@triceo triceo self-assigned this Jan 31, 2025
@triceo triceo had a problem deploying to documentation (preview) January 31, 2025 13:36 — with GitHub Actions Failure
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
89.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

zepfred
zepfred approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@zepfred zepfred left a comment

Choose a reason for hiding this comment

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

The changes have simplified the class's logic and look great to me!

@triceo
fix: prevent possible concurrent modification of a collection
7f723b0 
We have seen reports of the TreeMap getting concurrently accessed and throwing exceptions. Unfortunately, we have never seen any code to reproduce these failures, as concurrency issues are notoriously hard to reproduce. Therefore we refactor the mechanism to make any such issues impossible.

For the same reason as there is no reproducer, there is also no new test coverage. But existing test coverage still passes.
rsynek
rsynek approved these changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@rsynek rsynek left a comment

Choose a reason for hiding this comment

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

I like that a part of locking has been replaced by immutability, that's nice.

@triceo triceo merged commit a7c8374 into TimefoldAI:main Jan 31, 2025
26 of 27 checks passed
@triceo triceo deleted the threads branch January 31, 2025 15:57
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@rsynek rsynek rsynek approved these changes

@zepfred zepfred zepfred approved these changes

Assignees

@triceo triceo

Labels
None yet
Projects
None yet
Milestone
v1.19.0
Development

Successfully merging this pull request may close these issues.
3 participants
@triceo @rsynek @zepfred