Skip to content
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.

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

store element count as isize and clamp len to >= 0 #88

Merged
merged 1 commit into from
Apr 13, 2020

Conversation

domenicquirl
Copy link
Collaborator

@domenicquirl domenicquirl commented Apr 13, 2020

Changes the map's element count to be stored as a signed integer, to avoid panics if the count is updated by for example two removing and one inserting thread out of order.

In order to not report a negative size to users, len is adjusted to return 0 if the actual count is negative. This is translated from the Java implementation, but without the upper bounds check, since for us the storage data type (isize) cannot be larger than the maximal value of the return type (usize).

Note that this means that technically the same race condition still exists at the upper bound, however the map's MAXIMUM_CAPACITY is way below isize::MAX.

Closes #86.


This change is Reviewable

@codecov
Copy link

codecov bot commented Apr 13, 2020

Codecov Report

Merging #88 into master will decrease coverage by 0.00%.
The diff coverage is 85.71%.

Impacted Files Coverage Δ
src/node.rs 77.96% <ø> (ø)
src/map.rs 84.07% <85.71%> (-0.02%) ⬇️

@domenicquirl
Copy link
Collaborator Author

@jonhoo would you mind giving these changes another test run? I ran our tests on a branch with both this and #87 applied for a good half an hour and got no hits, but I also wasn't able to reproduce the errors this is supposed to fix before, so, yeah.

@jonhoo
Copy link
Owner

jonhoo commented Apr 13, 2020

Yup — I'll merge them both, and then run the test suite for a while. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

map::tree_bins::concurrent_tree_bin: attempt to subtract with overflow
2 participants