Fix creating a uniform snapshot from a collection #1111

Merged
merged 1 commit into from Mar 20, 2017

Conversation

Projects
None yet
3 participants
@arteam
Member

arteam commented Mar 19, 2017

The current code for creating a snapshot from the collection is unfortunately broken. The code fails if the provided collection is modified during creating of the snapshot and it has a weak iterator (precisely what ConcurrentSkipListMap provides). The iterator doesn't guarantee that it has exactly the same amount iteration as the size of the collection. As a result, the code throws an ArrayIndexOutOfBoundException.
The correct behaviour is too dump the collection to a resizable array on the fly and then convert it the array of long primitives.

The behaviour was correct in the 3.1 branch, but was broken in 3.2 by the optimization in #972.

Fixes #1104

@arteam arteam referenced this pull request Mar 19, 2017

Closed

Merge 3.2 to master #1110

Fix creating a uniform snapshot from a collection
The current code for creating a snapshot from the collection is unfortunately broken.
The code fails if the provided collection is modified during creating of the snapshot and
it has a weak iterator (precisely what `ConcurrentSkipListMap` provides). The iterator
doesn't guarantee that it has exactly the same amount iteration as the size of the
collection. As a result, the code throws an `ArrayIndexOutOfBoundException`.
The correct behaviour is too dump the collection to a resizable array on the fly and
then convert it the array of long primitives.

The behaviour was correct in the 3.1 branch, but was broken in 3.2 by the optimization
in #972.

@jplock jplock added this to the 3.2.2 milestone Mar 19, 2017

@jplock jplock added the bug label Mar 19, 2017

- int i = 0;
- for (Long value : values) {
- this.values[i++] = value;
+ final Object[] copy = values.toArray();

This comment has been minimized.

@jplock

jplock Mar 19, 2017

Member

can this be Long[] to avoid the cast below? Just curious

@jplock

jplock Mar 19, 2017

Member

can this be Long[] to avoid the cast below? Just curious

@arteam

This comment has been minimized.

Show comment
Hide comment
@arteam

arteam Mar 19, 2017

Member

It's possible to use the toArray version which is type-safe. I don't think there is much point, though. The array is then transformed to a primitive array and basically it's just a data dump. It would be great to avoid unnecessary copying, but I don't know a simple way to avoid it except changing the format of snapshot's storage from the array to the list.

Member

arteam commented Mar 19, 2017

It's possible to use the toArray version which is type-safe. I don't think there is much point, though. The array is then transformed to a primitive array and basically it's just a data dump. It would be great to avoid unnecessary copying, but I don't know a simple way to avoid it except changing the format of snapshot's storage from the array to the list.

@ryantenney

This comment has been minimized.

Show comment
Hide comment
@ryantenney

ryantenney Mar 19, 2017

Member

We might as well just revert #972, since toArray still results in copying the backing array.

Member

ryantenney commented Mar 19, 2017

We might as well just revert #972, since toArray still results in copying the backing array.

@arteam arteam merged commit bfd65f0 into 3.2-development Mar 20, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@arteam arteam deleted the fix_uniform_snapshot branch Mar 20, 2017

arteam added a commit to dropwizard/dropwizard that referenced this pull request Mar 20, 2017

Upgrade to metrics 3.2.2
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.*  branch: dropwizard/metrics#1111

@arteam arteam referenced this pull request in dropwizard/dropwizard Mar 20, 2017

Merged

Upgrade to metrics 3.2.2 #1970

joschi added a commit to dropwizard/dropwizard that referenced this pull request Mar 21, 2017

Upgrade to Metrics 3.2.2 (#1970)
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.x  branch: dropwizard/metrics#1111

joschi added a commit to dropwizard/dropwizard that referenced this pull request Mar 21, 2017

Upgrade to Metrics 3.2.2 (#1970)
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.x  branch: dropwizard/metrics#1111
(cherry picked from commit d28845d)

lucesape added a commit to Spirals-Team/librepair-experiments that referenced this pull request Mar 23, 2017

Upgrade to Metrics 3.2.2 (#1970)
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.x  branch: dropwizard/metrics#1111
(cherry picked from commit d28845d468f0577256b500b52ab8b5ee52b77508)

lucesape added a commit to Spirals-Team/librepair-experiments that referenced this pull request Mar 23, 2017

Upgrade to Metrics 3.2.2 (#1970)
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.x  branch: dropwizard/metrics#1111
(cherry picked from commit d28845d468f0577256b500b52ab8b5ee52b77508)

lucesape added a commit to Spirals-Team/librepair-experiments that referenced this pull request Mar 23, 2017

Upgrade to Metrics 3.2.2 (#1970)
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.x  branch: dropwizard/metrics#1111
(cherry picked from commit d28845d468f0577256b500b52ab8b5ee52b77508)

lucesape added a commit to surli/librepair-XP that referenced this pull request Apr 7, 2017

Upgrade to Metrics 3.2.2 (#1970)
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.x  branch: dropwizard/metrics#1111
(cherry picked from commit d28845d468f0577256b500b52ab8b5ee52b77508)

lucesape added a commit to surli/librepair-XP that referenced this pull request Apr 7, 2017

Upgrade to Metrics 3.2.2 (#1970)
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.x  branch: dropwizard/metrics#1111
(cherry picked from commit d28845d468f0577256b500b52ab8b5ee52b77508)

lucesape added a commit to surli/librepair-XP that referenced this pull request Apr 7, 2017

Upgrade to Metrics 3.2.2 (#1970)
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.x  branch: dropwizard/metrics#1111
(cherry picked from commit d28845d468f0577256b500b52ab8b5ee52b77508)

aaanders added a commit to aaanders/dropwizard that referenced this pull request Apr 26, 2017

Upgrade to Metrics 3.2.2 (#1970)
Release notes: https://github.com/dropwizard/metrics/releases/tag/v3.2.2

This release fixes a nasty bug during creating a snapshot which was introduced
in the 3.2.x  branch: dropwizard/metrics#1111
(cherry picked from commit d28845d)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment