ctubbsii commented on code in PR #3219:
URL: https://github.com/apache/accumulo/pull/3219#discussion_r1124711458
##########
core/src/main/java/org/apache/accumulo/core/file/blockfile/impl/SeekableByteArrayInputStream.java:
##########
@@ -94,13 +96,13 @@ public long skip(long requestedSkip) {
}
}
- cur += actualSkip;
+ cur.getAndAdd(actualSkip);
Review Comment:
The skip method is another one where the get and the add are disjointed, and
should be atomic.
##########
core/src/main/java/org/apache/accumulo/core/file/blockfile/impl/SeekableByteArrayInputStream.java:
##########
@@ -78,14 +80,14 @@ public int read(byte[] b, int offset, int length) {
length = avail;
}
- System.arraycopy(buffer, cur, b, offset, length);
- cur += length;
+ System.arraycopy(buffer, cur.get(), b, offset, length);
+ cur.getAndAdd(length);
Review Comment:
This section is another section where the value could change, and it's not
thread-safe, between the various non-atomic calls to `cur.get()`. There's one
hidden in the `available()` call, and then one in the array copy, then the
subsequent advancement of the pointer.
If we want this to be thread-safe, it should probably get the current value
and advance the pointer atomically, so that outside threads can't change the
pointer during the same call, similar to my previous suggestion.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
