You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2021/08/03 08:51:04 UTC
[GitHub] [commons-compress] matthijsln opened a new pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
matthijsln opened a new pull request #214:
URL: https://github.com/apache/commons-compress/pull/214
See https://issues.apache.org/jira/browse/COMPRESS-584
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892573540
Wrapping a byte array in a `ByteBuffer` is the most efficient. In the following implementation an array copy is only needed when hitting EOF:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final byte[] out = new byte[len];
final ByteBuffer b = ByteBuffer.wrap(out);
while (b.hasRemaining()) {
if (input.read(b) <= 0) {
break;
}
}
if (b.hasRemaining()) {
// We hit EOF and read less bytes, return a smaller array than len
return Arrays.copyOf(b.array(), b.position());
} else {
return out;
}
}
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892573540
Wrapping a byte array in a `ByteBuffer` is the most efficient. In the following implementation an array copy is only needed when hitting EOF:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final byte[] out = new byte[len];
final ByteBuffer b = ByteBuffer.wrap(out);
while (b.hasRemaining()) {
if (input.read(b) <= 0) {
break;
}
}
if (b.hasRemaining()) {
// We hit EOF and read less bytes, return a smaller array than len
return Arrays.copyOf(b.array(), b.position());
} else {
return out;
}
}
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892504852
This would work without a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteBuffer b = ByteBuffer.allocate(len);
while (b.hasRemaining()) {
if (input.read(b) <= 0) {
break;
}
}
return Arrays.copyOf(b.array(), b.position());
}
```
With a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteArrayOutputStream output = new ByteArrayOutputStream();
final ByteBuffer b = ByteBuffer.allocate(Math.min(len, COPY_BUF_SIZE));
int read = 0;
while (read < len) {
// Make sure we never read more than len bytes
b.limit(Math.min(len - read, b.capacity()));
final int readNow = input.read(b);
if (readNow <= 0) {
break;
}
output.write(b.array(), 0, readNow);
b.rewind();
read += readNow;
}
return output.toByteArray();
}
```
Additional test case for reading more than the copy buffer length (of course COPY_BUF_SIZE can be made more accessible instead of using reflection):
```
@Test
public void readRangeMoreThanCopyBufferSize() throws Exception {
final Field COPY_BUF_SIZE = IOUtils.class.getDeclaredField("COPY_BUF_SIZE");
COPY_BUF_SIZE.setAccessible(true);
final int copyBufSize = (int)COPY_BUF_SIZE.get(null);
// Make an input that requires two read loops
byte[] input = new byte[copyBufSize + 10];
try (SeekableByteChannel in = new SeekableInMemoryByteChannel(input)) {
// Ask for less than the input length, but more than the buffer size
int toRead = copyBufSize + 1;
byte[] read = IOUtils.readRange(in, toRead);
assertEquals(toRead, read.length);
assertEquals(toRead, in.position());
}
}
```
Hope this is correct :) What version is preferable? I don't think a copy buffer in itself is needed and maximum memory requirements with a copy buffer is at least `len * 2 + min(len,COPY_BUF_SIZE)` bytes as `ByteArrayOutputStream.toByteArray()` eventually calls `Arrays.copyOf(byte[],int)` which allocates a new byte array. The no copy buffer version only requires `len * 2` bytes.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892485686
> > Note that for [COMPRESS-585](https://issues.apache.org/jira/browse/COMPRESS-585) I propose removing the limit to the max bytes read to COPY_BUF_SIZE altogether (the same as the `IOUtils.readRange(InputStream,int)` method) because the calling code does not expect the method to return less than `len` bytes unless EOF is encountered.
> > With the limit removed we can use the [f8587ff](https://github.com/apache/commons-compress/commit/f8587ff16469bcda58db57baafabc9e4639e7f93) version.
>
> No. The `COPY_BUF_SIZE` is not a limit. It's just a limit of the buffer's size.
> Note that the `ByteBuffer b` is just a buffer for copying the bytes. That's why the `rewind` is nessary here - we can not always ensure the `len` is less than `COPY_BUF_SIZE`.
Ah, I think I get it :) The problem in COMPRESS-585 is the same as COMPRESS-584 then. The original intent of the method is to return a byte array of `len` bytes or less if EOF was encountered. I mistakenly thought it was designed to return max COPY_BUF_SIZE even of no EOF.
To summarize:
- If the first `read()` call returned less than `len`, for the next `read()` call it could read more than `len`. This can happen reading from a socket.
- If `len` is more than COPY_BUF_SIZE, it would read in more loops and may read more than `len`. This can happen with an entry comment longer than COPY_BUF_SIZE.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on a change in pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on a change in pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#discussion_r682349198
##########
File path: src/test/java/org/apache/commons/compress/utils/IOUtilsTest.java
##########
@@ -148,6 +148,25 @@ public void readRangeFromChannelDoesntReadMoreThanAskedFor() throws IOException
}
}
+ @Test
+ public void readRangeFromChannelDoesntReadMoreThanAskedForWhenItGotLessInFirstReadCall() throws IOException {
+ try (ReadableByteChannel in = new SeekableInMemoryByteChannel(new byte[] { 1, 2, 3, 4, 5 }) {
+ @Override
+ public int read(ByteBuffer buf) throws IOException {
+ // Read max 3 bytes at a time
+ ByteBuffer temp = ByteBuffer.allocate(Math.min(3, buf.remaining()));
Review comment:
I think we can have a `final` here.
Even through this is just a test, it's always appreciated if `final` is always used if possible.
##########
File path: src/test/java/org/apache/commons/compress/utils/IOUtilsTest.java
##########
@@ -148,6 +148,25 @@ public void readRangeFromChannelDoesntReadMoreThanAskedFor() throws IOException
}
}
+ @Test
+ public void readRangeFromChannelDoesntReadMoreThanAskedForWhenItGotLessInFirstReadCall() throws IOException {
+ try (ReadableByteChannel in = new SeekableInMemoryByteChannel(new byte[] { 1, 2, 3, 4, 5 }) {
+ @Override
+ public int read(ByteBuffer buf) throws IOException {
+ // Read max 3 bytes at a time
+ ByteBuffer temp = ByteBuffer.allocate(Math.min(3, buf.remaining()));
+ int read = super.read(temp);
+ if (read > 0) {
+ buf.put(temp.array(), 0, read);
+ }
+ return read;
+ }
+ }) {
+ byte[] read = IOUtils.readRange(in, 4);
Review comment:
Same as above
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892504852
This would work without a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteBuffer b = ByteBuffer.allocate(len);
while (b.hasRemaining()) {
if (input.read(b) <= 0) {
break;
}
}
return Arrays.copyOf(b.array(), b.position());
}
```
With a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteArrayOutputStream output = new ByteArrayOutputStream();
final ByteBuffer b = ByteBuffer.allocate(Math.min(len, COPY_BUF_SIZE));
int read = 0;
while (read < len) {
// Make sure we never read more than len bytes
b.limit(Math.min(len - read, b.capacity()));
final int readNow = input.read(b);
if (readNow <= 0) {
break;
}
output.write(b.array(), 0, readNow);
b.rewind();
read += readNow;
}
return output.toByteArray();
}
```
Additional test case for reading more than the copy buffer length (of course COPY_BUF_SIZE can be made more accessible instead of using reflection):
```
@Test
public void readRangeMoreThanCopyBufferSize() throws Exception {
final Field COPY_BUF_SIZE = IOUtils.class.getDeclaredField("COPY_BUF_SIZE");
COPY_BUF_SIZE.setAccessible(true);
final int copyBufSize = (int)COPY_BUF_SIZE.get(null);
// Make an input that requires two read loops
byte[] input = new byte[copyBufSize + 10];
try (SeekableByteChannel in = new SeekableInMemoryByteChannel(input)) {
// Ask for less than the input length, but more than the buffer size
int toRead = copyBufSize + 1;
byte[] read = IOUtils.readRange(in, toRead);
assertEquals(toRead, read.length);
assertEquals(toRead, in.position());
}
}
```
Hope this is correct :) What version is preferable? I don't think a copy buffer in itself is needed, although the maximum memory requirement is `len * 2` instead of `len + COPY_BUF_SIZE`.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892573540
The "no copy buffer" version can be more efficient by only calling `Arrays.copyOf()` when hitting EOF. This avoids copying the byte array when it is completely filled:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteBuffer b = ByteBuffer.allocate(len);
while (b.hasRemaining()) {
if (input.read(b) <= 0) {
break;
}
}
if (b.hasRemaining()) {
// We hit EOF and read less bytes, return a smaller array than len
return Arrays.copyOf(b.array(), b.position());
} else {
return b.array();
}
}
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892504852
This would work without a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteBuffer b = ByteBuffer.allocate(len);
while (b.hasRemaining()) {
if (input.read(b) <= 0) {
break;
}
}
return Arrays.copyOfRange(b.array(), 0, b.position());
}
```
With a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteArrayOutputStream output = new ByteArrayOutputStream();
final ByteBuffer b = ByteBuffer.allocate(Math.min(len, COPY_BUF_SIZE));
int read = 0;
while (read < len) {
// Make sure we never read more than len bytes
b.limit(Math.min(len - read, b.remaining()));
final int readNow = input.read(b);
if (readNow <= 0) {
break;
}
output.write(b.array(), 0, readNow);
b.rewind();
read += readNow;
}
return output.toByteArray();
}
```
Additional test case for reading more than the copy buffer length (of course COPY_BUF_SIZE can be made more accessible instead of using reflection):
```
@Test
public void readRangeMoreThanCopyBufferSize() throws Exception {
final Field COPY_BUF_SIZE = IOUtils.class.getDeclaredField("COPY_BUF_SIZE");
COPY_BUF_SIZE.setAccessible(true);
final int copyBufSize = (int)COPY_BUF_SIZE.get(null);
// Make an input that requires two read loops
byte[] input = new byte[copyBufSize + 10];
try (SeekableByteChannel in = new SeekableInMemoryByteChannel(input)) {
// Ask for less than the input length, but more than the buffer size
int toRead = copyBufSize + 1;
byte[] read = IOUtils.readRange(in, toRead);
assertEquals(toRead, read.length);
assertEquals(toRead, in.position());
}
}
```
Hope this is correct :) What version is preferable? I don't think a copy buffer in itself is needed.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-896453094
Hi @matthijsln
I want to merge this PR first. The current implementation didn't change much from original implemention.
I will investigate into the "no copy buffer" implementation later when I can curve out some time.
Thank you for your contributions!
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892469732
> Hi @matthijsln
> Please have a look at my reply :-)
>
> > When the len of bytes to read is bigger than COPY_BUF_SIZE, there are some problems. You can reproduce this by simply set the COPY_BUF_SIZE to a small value.
Indeed it did not handle a small COPY_BUF_SIZE correctly. If I set it to 1, the test cases fail because they do not expect it, but with the e50dedf version it never reads more than COPY_BUF_SIZE.
Note that in [COMPRESS-585](https://issues.apache.org/jira/browse/COMPRESS-585) I propose limiting the number of bytes read to COPY_BUF_SIZE altogether (the same as the `IOUtils.readRange(InputStream,int)` method) because the calling code does not expect the method to return less than `len` bytes unless EOF is encountered.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892469732
> Hi @matthijsln
> Please have a look at my reply :-)
>
> > When the len of bytes to read is bigger than COPY_BUF_SIZE, there are some problems. You can reproduce this by simply set the COPY_BUF_SIZE to a small value.
Indeed it did not handle a small COPY_BUF_SIZE correctly. If I set it to 1, the test cases fail because they do not expect it, but with the e50dedf version it never reads more than COPY_BUF_SIZE and returns correctly.
Note that for [COMPRESS-585](https://issues.apache.org/jira/browse/COMPRESS-585) I propose removing the limit to the max bytes read to COPY_BUF_SIZE altogether (the same as the `IOUtils.readRange(InputStream,int)` method) because the calling code does not expect the method to return less than `len` bytes unless EOF is encountered.
With the limit removed we can use the f8587ff version.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892526508
I prefer the one with a copy buffer.
I'm not professional in IO but the implemention with buffer is pretty common in IO implementions.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee merged pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee merged pull request #214:
URL: https://github.com/apache/commons-compress/pull/214
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892504852
This would work without a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteBuffer b = ByteBuffer.allocate(len);
while (b.hasRemaining()) {
if (input.read(b) <= 0) {
break;
}
}
return Arrays.copyOfRange(b.array(), 0, b.position());
}
```
With a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteArrayOutputStream output = new ByteArrayOutputStream();
final ByteBuffer b = ByteBuffer.allocate(Math.min(len, COPY_BUF_SIZE));
int read = 0;
while (read < len) {
// Make sure we never read more than len bytes
b.limit(Math.min(len - read, b.capacity()));
final int readNow = input.read(b);
if (readNow <= 0) {
break;
}
output.write(b.array(), 0, readNow);
b.rewind();
read += readNow;
}
return output.toByteArray();
}
```
Additional test case for reading more than the copy buffer length (of course COPY_BUF_SIZE can be made more accessible instead of using reflection):
```
@Test
public void readRangeMoreThanCopyBufferSize() throws Exception {
final Field COPY_BUF_SIZE = IOUtils.class.getDeclaredField("COPY_BUF_SIZE");
COPY_BUF_SIZE.setAccessible(true);
final int copyBufSize = (int)COPY_BUF_SIZE.get(null);
// Make an input that requires two read loops
byte[] input = new byte[copyBufSize + 10];
try (SeekableByteChannel in = new SeekableInMemoryByteChannel(input)) {
// Ask for less than the input length, but more than the buffer size
int toRead = copyBufSize + 1;
byte[] read = IOUtils.readRange(in, toRead);
assertEquals(toRead, read.length);
assertEquals(toRead, in.position());
}
}
```
Hope this is correct :) What version is preferable? I don't think a copy buffer in itself is needed.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892459986
The new implemention also has some problems :
1. The test `readRangeFromChannelStopsIfThereIsNothingToReadAnymore` is failed
2. When the `len` of bytes to read is bigger than `COPY_BUF_SIZE`, there are some problems. You can reproduce this by simply set the `COPY_BUF_SIZE` to a small value.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892469732
> Hi @matthijsln
> Please have a look at my reply :-)
>
> > When the len of bytes to read is bigger than COPY_BUF_SIZE, there are some problems. You can reproduce this by simply set the COPY_BUF_SIZE to a small value.
Indeed it did not handle a small COPY_BUF_SIZE correctly. If I set it to 1, the test cases fail because they do not expect it, but with the e50dedf version it never reads more than COPY_BUF_SIZE and returns correctly.
Note that for [COMPRESS-585](https://issues.apache.org/jira/browse/COMPRESS-585) I propose removing the COPY_BUF_SIZE limit to the max bytes to read altogether (the same as the `IOUtils.readRange(InputStream,int)` method) because the calling code does not expect the method to return less than `len` bytes unless EOF is encountered.
With the limit removed we can use the f8587ff version.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-891708225
[](https://coveralls.io/builds/41898328)
Coverage increased (+0.0005%) to 86.355% when pulling **cf5adfba2e3f8a1eebb51af817234bd50e99f734 on B3Partners:COMPRESS-584** into **8287bfdb8adbeae433e0a1205c6e4fb180b9d3be on apache:master**.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892460047
> Unfortunately my original fix had a bug when looping more than 2 times. Revisiting this method I came up with a much simpler implementation that should be better. I can squash the previous version and force-push if this looks better (or it can be squash-merged).
Third time's a charm...
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892478291
> Note that for [COMPRESS-585](https://issues.apache.org/jira/browse/COMPRESS-585) I propose removing the limit to the max bytes read to COPY_BUF_SIZE altogether (the same as the `IOUtils.readRange(InputStream,int)` method) because the calling code does not expect the method to return less than `len` bytes unless EOF is encountered.
>
> With the limit removed we can use the [f8587ff](https://github.com/apache/commons-compress/commit/f8587ff16469bcda58db57baafabc9e4639e7f93) version.
No. The `COPY_BUF_SIZE` is not a limit. It's just a limit of the buffer's size.
Note that the `ByteBuffer b` is just a buffer for copying the bytes. That's why the `rewind` is nessary here - we can not always ensure the `len` is less than `COPY_BUF_SIZE`.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-893209493
This current implemention looks good to me now.
For the "no copy buffer" version, it looks more efficient. But I'm not quite sure about this. I will try to make some benchmark tests if I got time. And I'd like to hear ideas from someone else.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892504852
This would work without a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteBuffer b = ByteBuffer.allocate(len);
while (b.hasRemaining()) {
if (input.read(b) <= 0) {
break;
}
}
return Arrays.copyOf(b.array(), b.position());
}
```
With a copy buffer:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteArrayOutputStream output = new ByteArrayOutputStream();
final ByteBuffer b = ByteBuffer.allocate(Math.min(len, COPY_BUF_SIZE));
int read = 0;
while (read < len) {
// Make sure we never read more than len bytes
b.limit(Math.min(len - read, b.capacity()));
final int readNow = input.read(b);
if (readNow <= 0) {
break;
}
output.write(b.array(), 0, readNow);
b.rewind();
read += readNow;
}
return output.toByteArray();
}
```
Additional test case for reading more than the copy buffer length (of course COPY_BUF_SIZE can be made more accessible instead of using reflection):
```
@Test
public void readRangeMoreThanCopyBufferSize() throws Exception {
final Field COPY_BUF_SIZE = IOUtils.class.getDeclaredField("COPY_BUF_SIZE");
COPY_BUF_SIZE.setAccessible(true);
final int copyBufSize = (int)COPY_BUF_SIZE.get(null);
// Make an input that requires two read loops
byte[] input = new byte[copyBufSize + 10];
try (SeekableByteChannel in = new SeekableInMemoryByteChannel(input)) {
// Ask for less than the input length, but more than the buffer size
int toRead = copyBufSize + 1;
byte[] read = IOUtils.readRange(in, toRead);
assertEquals(toRead, read.length);
assertEquals(toRead, in.position());
}
}
```
Hope this is correct :) What version is preferable? I don't think a copy buffer in itself is needed and maximum memory requirements with a copy buffer is at least `len * 2 + COPY_BUF_SIZE` bytes as `ByteArrayOutputStream.toByteArray()` eventually calls `Arrays.copyOf(byte[],int)` which allocates a new byte array. The no copy buffer version only requires `len * 2` bytes.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892573540
The "no copy buffer" version can be more efficient by only calling `Arrays.copyOf()` when hitting EOF. This avoids copying the byte array when it is completely filled:
```
public static byte[] readRange(final ReadableByteChannel input, final int len) throws IOException {
final ByteBuffer b = ByteBuffer.allocate(len);
while (b.hasRemaining()) {
if (input.read(b) <= 0) {
break;
}
}
if (b.hasRemaining()) {
// We hit EOF and read less bytes, return a smaller array than len
return Arrays.copyOf(b.array(), b.position());
} else {
return b.array();
}
}
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-891708225
[](https://coveralls.io/builds/41898328)
Coverage increased (+0.0005%) to 86.355% when pulling **cf5adfba2e3f8a1eebb51af817234bd50e99f734 on B3Partners:COMPRESS-584** into **8287bfdb8adbeae433e0a1205c6e4fb180b9d3be on apache:master**.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892450747
Unfortunately my original fix had a bug when looping more than 2 times. Revisiting this method I came up with a much simpler implementation that should be better. I can squash the previous version and force-push if this looks better (or it can be squash-merged).
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892419298
Good catch!
This looks good to me.
Thanks for your contributions.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] coveralls commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
coveralls commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-891708225
[](https://coveralls.io/builds/41859837)
Coverage increased (+0.001%) to 86.355% when pulling **fb72646401d1da599d6b7d68acc04c50a02f4d66 on B3Partners:COMPRESS-584** into **8287bfdb8adbeae433e0a1205c6e4fb180b9d3be on apache:master**.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892469732
> Hi @matthijsln
> Please have a look at my reply :-)
>
> > When the len of bytes to read is bigger than COPY_BUF_SIZE, there are some problems. You can reproduce this by simply set the COPY_BUF_SIZE to a small value.
Indeed it did not handle a small COPY_BUF_SIZE correctly. If I set it to 1, the test cases fail because they do not expect it, but with the e50dedf version it never reads more than COPY_BUF_SIZE and returns correctly.
Note that for [COMPRESS-585](https://issues.apache.org/jira/browse/COMPRESS-585) I propose removing the limit to the max bytes read to COPY_BUF_SIZE altogether (the same as the `IOUtils.readRange(InputStream,int)` method) because the calling code does not expect the method to return less than `len` bytes unless EOF is encountered.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892526508
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] PeterAlfredLee commented on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
PeterAlfredLee commented on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892466407
Hi @matthijsln
Please have a look at my reply :-)
> When the len of bytes to read is bigger than COPY_BUF_SIZE, there are some problems. You can reproduce this by simply set the COPY_BUF_SIZE to a small value.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-compress] matthijsln edited a comment on pull request #214: [COMPRESS-584] Fix IOUtils.readRange() can read more from a channel than asked for
Posted by GitBox <gi...@apache.org>.
matthijsln edited a comment on pull request #214:
URL: https://github.com/apache/commons-compress/pull/214#issuecomment-892469732
> Hi @matthijsln
> Please have a look at my reply :-)
>
> > When the len of bytes to read is bigger than COPY_BUF_SIZE, there are some problems. You can reproduce this by simply set the COPY_BUF_SIZE to a small value.
Indeed it did not handle a small COPY_BUF_SIZE correctly. If I set it to 1, the test cases fail because they do not expect it, but with the e50dedf version it never reads more than COPY_BUF_SIZE and returns correctly.
Note that in [COMPRESS-585](https://issues.apache.org/jira/browse/COMPRESS-585) I propose limiting the number of bytes read to COPY_BUF_SIZE altogether (the same as the `IOUtils.readRange(InputStream,int)` method) because the calling code does not expect the method to return less than `len` bytes unless EOF is encountered.
--
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: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org