Fixes an ArrayIndexOutOfBounds issue iterating over relationships #1011

Merged
merged 1 commit into from Aug 8, 2013

Conversation

Projects
None yet
2 participants
Owner

tinwelint commented Jul 28, 2013

Issue manifests in calling #next() on the iterator gotten from
Node#getRelationships(...) would suddenly throw ArrayIndexOutOfBoundsException.

Cause of the issue was this: Internally a RelationshipIterator would have
all internal per-type relationship iterators in an array. Exhausting one
would try to move to the next, if any. The check that tested if there were
more per-type iterators to visit incremented the array index and if that
check would yield false an additional check to load more relationships
would be made, and if that also yielded false the current per-type iterator
would be tested again. In this case another transaction would have created
another relationship so that relationship would be returned.
The consecutive call to #next() would start off getting the current
per-type iterator and this is where the array index would point to an
index that was one too high than allowed.

Fix is to not increment the index in the check, but instead increment if
the check yields true.

Owner

tinwelint commented Jul 28, 2013

Fixes #880

Owner

thobe commented Jul 30, 2013

The code is a bit hard to follow, but the commit message makes the intent pretty clear. That message is not going to stay with the code though, I wonder if the code could be made clearer somehow. The comments help, but it still feels hard to follow.

Could the method perhaps be broken down into smaller easier-to-follow methods with names that make the behaviour super clear?

I'm also a bit sad that there isn't a deterministic test for this, although I can appreciate that such a test is hard to write.

Owner

tinwelint commented Jul 30, 2013

I agree that it's hard to follow the execution by reading it. Refactorings should be done in 2.0 so that could be something that we do there after this commit has been merged over. It should definitely be possible to make things clearer by breaking down into smaller, well named methods.

I'm not against having the added test in as it doesn't just tests this specific issue, but other potential issues like it. It does make sense to add one more deterministic unit test for this.

Owner

thobe commented Jul 31, 2013

@tinwelint do you think it would be possible to write such a deterministic unit test? Do you have an outline of how it would be done? How much work do you think it would be? With the current test, with what probability did you find that it triggered the issue?

Owner

tinwelint commented Jul 31, 2013

@thobe yeah I think it should be possible and I've got an idea of how. The test I wrote triggered the issue 100% of the times within the first 1% of the maximum time (0.5 seconds) given to it. However we've seen that tests behave vastly different on different machines, especially some CI setups, so it may be less reliable elsewhere.

Fixes an ArrayIndexOutOfBounds issue iterating over relationships
Issue manifests in calling #next() on the iterator gotten from
Node#getRelationships(...) would suddenly throw ArrayIndexOutOfBoundsException.

Cause of the issue was this: Internally a RelationshipIterator would have
all internal per-type relationship iterators in an array. Exhausting one
would try to move to the next, if any. The check that tested if there were
more per-type iterators to visit incremented the array index and if that
check would yield false an additional check to load more relationships
would be made, and if that also yielded false the current per-type iterator
would be tested again. In this case another transaction would have created
another relationship so that relationship would be returned.
  The consecutive call to #next() would start off getting the current
per-type iterator and this is where the array index would point to an
index that was one too high than allowed.

Fix is to not increment the index in the check, but instead increment if
the check yields true.
Owner

tinwelint commented Aug 6, 2013

@thobe I updated this PR with a unit test to go with it, worked well

thobe added a commit that referenced this pull request Aug 8, 2013

Merge pull request #1011 from tinwelint/1.9-fix-aioobe
Fixes an ArrayIndexOutOfBounds issue iterating over relationships

@thobe thobe merged commit aae9b9f into neo4j:1.9-maint Aug 8, 2013

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment