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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix arithmetic overflow at all default reservoir #753

Open
wants to merge 1 commit into
base: features/4.4.0
Choose a base branch
from

Conversation

MichaelAlekseev
Copy link

Thanks for helping out 馃憤

Before submitting a pull request, please have a quick read through the contribution guidelines and provide the following information, where appropriate replace the [ ] with a [X]

The issue or feature being addressed

  • Alreary was the same PR Added fix for arithmetic overflow at DefaultAlgorithmRReservoir聽#632. But it fix only DefaultAlgorithmRReservoir. Same problem exists for other reservoirs.
  • When reservoir processed more than int.MaxValue elements, this unchecked cast will result in negative value of a size.
    During snapshot creation array of UserValueWrappers will be created with negative size, thus resulting into OverflowException.
    Now during casting total count to int it will be always lower than int.MaxValue, because it is compared to value's size.

Details on the issue fix or feature implementation

  • Added base abstract generic reservoir ReservoirBase with fixed problem.
  • All DefaultXXXReservoir now inherited from ReservoirBase
  • Some members now common for ReservoirBase so simple refactored all DefaultXXXReservoir
  • Added new test that check fix this problem

Confirm the following

  • I have ensured that I have merged the latest changes from the dev branch
  • I have successfully run a local build
  • I have included unit tests for the issue/feature
  • I have included the github issue number in my commits

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.

None yet

1 participant