From 7c042b8d677ee3019de72e7e5c34c4d78d375ef9 Mon Sep 17 00:00:00 2001 From: Thomas Mortagne Date: Wed, 7 Feb 2018 11:04:53 +0100 Subject: [PATCH 1/5] IO-568: AutoCloseInputStream crash on reset() when reading the whole stream --- .../io/input/AutoCloseInputStream.java | 38 ++++++++++++++-- .../io/input/AutoCloseInputStreamTest.java | 44 +++++++++++++++++-- 2 files changed, 74 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java b/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java index e946b54e055..a657512e96b 100644 --- a/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java +++ b/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java @@ -16,11 +16,11 @@ */ package org.apache.commons.io.input; -import static org.apache.commons.io.IOUtils.EOF; - import java.io.IOException; import java.io.InputStream; +import static org.apache.commons.io.IOUtils.EOF; + /** * Proxy stream that closes and discards the underlying stream as soon as the * end of input has been reached or when the stream is explicitly closed. @@ -37,6 +37,8 @@ */ public class AutoCloseInputStream extends ProxyInputStream { + private boolean marked; + /** * Creates an automatically closing proxy for the given input stream. * @@ -66,7 +68,7 @@ public void close() throws IOException { } /** - * Automatically closes the stream if the end of stream was reached. + * Automatically closes the stream if the end of stream was reached unless the stream was marked. * * @param n number of bytes read, or -1 if no more bytes are available * @throws IOException if the stream could not be closed @@ -74,11 +76,39 @@ public void close() throws IOException { */ @Override protected void afterRead(final int n) throws IOException { - if (n == EOF) { + if (n == EOF && !marked) { close(); } } + /** + * {@inheritDoc} + *

+ * Make sure to remember that the stream was makred to not close it when reaching the end. + * + * @see org.apache.commons.io.input.ProxyInputStream#mark(int) + */ + @Override + public synchronized void mark(int readlimit) { + super.mark(readlimit); + + marked = true; + } + + /** + * {@inheritDoc} + *

+ * Reset the marked flag. + * + * @see org.apache.commons.io.input.ProxyInputStream#reset() + */ + @Override + public synchronized void reset() throws IOException { + super.reset(); + + marked = false; + } + /** * Ensures that the stream is closed before it gets garbage-collected. * As mentioned in {@link #close()}, this is a no-op if the stream has diff --git a/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java b/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java index 09c4e9f5b9d..646dee8dbc6 100644 --- a/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java +++ b/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java @@ -16,10 +16,6 @@ */ package org.apache.commons.io.input; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - import java.io.ByteArrayInputStream; import java.io.IOException; import java.io.InputStream; @@ -27,6 +23,10 @@ import org.junit.Before; import org.junit.Test; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + /** * JUnit Test Case for {@link AutoCloseInputStream}. */ @@ -100,4 +100,40 @@ public void testReadBufferOffsetLength() throws IOException { assertEquals("read(b, off, len)", -1, stream.read(b, 0, b.length)); } + @Test + public void testMark() throws IOException + { + this.stream.mark(4); + + assertEquals('x', this.stream.read()); + assertEquals('y', this.stream.read()); + + this.stream.reset(); + + assertEquals('x', this.stream.read()); + assertEquals('y', this.stream.read()); + } + + @Test + public void testMarkFullStream() throws IOException + { + this.stream.mark(4); + + assertEquals('x', this.stream.read()); + assertEquals('y', this.stream.read()); + assertEquals('z', this.stream.read()); + assertEquals(-1, this.stream.read()); + + assertFalse(closed); + + this.stream.reset(); + + assertEquals('x', this.stream.read()); + assertEquals('y', this.stream.read()); + assertEquals('z', this.stream.read()); + assertEquals(-1, this.stream.read()); + + assertTrue(closed); + } + } From 56d2264f7655bc1e6c62ef750531492dcb9e8297 Mon Sep 17 00:00:00 2001 From: Thomas Mortagne Date: Thu, 8 Feb 2018 17:58:57 +0100 Subject: [PATCH 2/5] IO-568: AutoCloseInputStream should disable mark/reset --- .../io/input/AutoCloseInputStream.java | 50 ++++++++++-------- .../io/input/AutoCloseInputStreamTest.java | 51 +++++++------------ 2 files changed, 47 insertions(+), 54 deletions(-) diff --git a/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java b/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java index a657512e96b..b206ad145b3 100644 --- a/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java +++ b/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java @@ -16,29 +16,27 @@ */ package org.apache.commons.io.input; +import static org.apache.commons.io.IOUtils.EOF; + import java.io.IOException; import java.io.InputStream; -import static org.apache.commons.io.IOUtils.EOF; - /** - * Proxy stream that closes and discards the underlying stream as soon as the - * end of input has been reached or when the stream is explicitly closed. - * Not even a reference to the underlying stream is kept after it has been - * closed, so any allocated in-memory buffers can be freed even if the - * client application still keeps a reference to the proxy stream. + * Proxy stream that closes and discards the underlying stream as soon as the end of input has been reached or when the + * stream is explicitly closed. Not even a reference to the underlying stream is kept after it has been closed, so any + * allocated in-memory buffers can be freed even if the client application still keeps a reference to the proxy stream. *

- * This class is typically used to release any resources related to an open - * stream as soon as possible even if the client application (by not explicitly - * closing the stream when no longer needed) or the underlying stream (by not + * This class is typically used to release any resources related to an open stream as soon as possible even if the + * client application (by not explicitly closing the stream when no longer needed) or the underlying stream (by not * releasing resources once the last byte has been read) do not do that. + *

+ * Since the only way to always close the stream when reaching the end and respecting mark/reset contract, + * AutoCloseInputStream force disabling mark/reset. * * @since 1.4 */ public class AutoCloseInputStream extends ProxyInputStream { - private boolean marked; - /** * Creates an automatically closing proxy for the given input stream. * @@ -68,7 +66,7 @@ public void close() throws IOException { } /** - * Automatically closes the stream if the end of stream was reached unless the stream was marked. + * Automatically closes the stream if the end of stream was reached. * * @param n number of bytes read, or -1 if no more bytes are available * @throws IOException if the stream could not be closed @@ -76,7 +74,7 @@ public void close() throws IOException { */ @Override protected void afterRead(final int n) throws IOException { - if (n == EOF && !marked) { + if (n == EOF) { close(); } } @@ -84,29 +82,39 @@ protected void afterRead(final int n) throws IOException { /** * {@inheritDoc} *

- * Make sure to remember that the stream was makred to not close it when reaching the end. + * AutoCloseInputStream does not support mark/reset no matter what. * * @see org.apache.commons.io.input.ProxyInputStream#mark(int) */ @Override public synchronized void mark(int readlimit) { - super.mark(readlimit); - - marked = true; + // Behave as standard InputStream not supporting mark/reset } /** * {@inheritDoc} *

- * Reset the marked flag. + * AutoCloseInputStream does not support mark/reset no matter what. * * @see org.apache.commons.io.input.ProxyInputStream#reset() */ @Override public synchronized void reset() throws IOException { - super.reset(); + // Behave as standard InputStream not supporting mark/reset + throw new IOException("mark/reset not supported"); + } - marked = false; + /** + * {@inheritDoc} + *

+ * AutoCloseInputStream does not support mark/reset no matter what. + * + * @see org.apache.commons.io.input.ProxyInputStream#markSupported() + */ + @Override + public boolean markSupported() { + // Behave as standard InputStream not supporting mark/reset + return false; } /** diff --git a/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java b/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java index 646dee8dbc6..c70846d1672 100644 --- a/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java +++ b/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java @@ -20,6 +20,7 @@ import java.io.IOException; import java.io.InputStream; +import org.apache.commons.io.serialization.MockSerializedClass; import org.junit.Before; import org.junit.Test; @@ -34,6 +35,8 @@ public class AutoCloseInputStreamTest { private byte[] data; + private InputStream targetStream; + private InputStream stream; private boolean closed; @@ -41,12 +44,13 @@ public class AutoCloseInputStreamTest { @Before public void setUp() { data = new byte[] { 'x', 'y', 'z' }; - stream = new AutoCloseInputStream(new ByteArrayInputStream(data) { + targetStream = new ByteArrayInputStream(data) { @Override public void close() { closed = true; } - }); + }; + stream = new AutoCloseInputStream(targetStream); closed = false; } @@ -103,37 +107,18 @@ public void testReadBufferOffsetLength() throws IOException { @Test public void testMark() throws IOException { - this.stream.mark(4); - - assertEquals('x', this.stream.read()); - assertEquals('y', this.stream.read()); - - this.stream.reset(); - - assertEquals('x', this.stream.read()); - assertEquals('y', this.stream.read()); - } - - @Test - public void testMarkFullStream() throws IOException - { - this.stream.mark(4); - - assertEquals('x', this.stream.read()); - assertEquals('y', this.stream.read()); - assertEquals('z', this.stream.read()); - assertEquals(-1, this.stream.read()); - - assertFalse(closed); - - this.stream.reset(); - - assertEquals('x', this.stream.read()); - assertEquals('y', this.stream.read()); - assertEquals('z', this.stream.read()); - assertEquals(-1, this.stream.read()); - - assertTrue(closed); + assertTrue(targetStream.markSupported()); + + // Make sure mark is disabled + assertFalse(stream.markSupported()); + // Check that mark() does not fail + stream.mark(1); + // Check that reset() throw an exception + try { + stream.reset(); + } catch (IOException expected) { + assertEquals("mark/reset not supported", expected.getMessage()); + } } } From 508c6ab21bf6c5ae7c2f028c8a7c651c393928a4 Mon Sep 17 00:00:00 2001 From: Thomas Mortagne Date: Fri, 9 Feb 2018 16:31:45 +0100 Subject: [PATCH 3/5] IO-568: AutoCloseInputStream should disable mark/reset --- .../commons/io/input/AutoCloseInputStream.java | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java b/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java index b206ad145b3..22eb8e04b4f 100644 --- a/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java +++ b/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java @@ -22,16 +22,20 @@ import java.io.InputStream; /** - * Proxy stream that closes and discards the underlying stream as soon as the end of input has been reached or when the - * stream is explicitly closed. Not even a reference to the underlying stream is kept after it has been closed, so any - * allocated in-memory buffers can be freed even if the client application still keeps a reference to the proxy stream. + * Proxy stream that closes and discards the underlying stream as soon as the + * end of input has been reached or when the stream is explicitly closed. + * Not even a reference to the underlying stream is kept after it has been + * closed, so any allocated in-memory buffers can be freed even if the + * client application still keeps a reference to the proxy stream. *

- * This class is typically used to release any resources related to an open stream as soon as possible even if the - * client application (by not explicitly closing the stream when no longer needed) or the underlying stream (by not + * This class is typically used to release any resources related to an open + * stream as soon as possible even if the client application (by not explicitly + * closing the stream when no longer needed) or the underlying stream (by not * releasing resources once the last byte has been read) do not do that. *

- * Since the only way to always close the stream when reaching the end and respecting mark/reset contract, - * AutoCloseInputStream force disabling mark/reset. + * Since the only way to always close the stream when reaching the end and + * respecting mark/reset contract, AutoCloseInputStream force disabling + * mark/reset. * * @since 1.4 */ From 2b1cdb3ea86eca432ec5eed7185c1c5447508207 Mon Sep 17 00:00:00 2001 From: Thomas Mortagne Date: Fri, 9 Feb 2018 16:36:49 +0100 Subject: [PATCH 4/5] IO-568: AutoCloseInputStream should disable mark/reset --- .../io/input/AutoCloseInputStreamTest.java | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java b/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java index c70846d1672..47b93527059 100644 --- a/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java +++ b/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java @@ -41,6 +41,10 @@ public class AutoCloseInputStreamTest { private boolean closed; + private boolean marked; + + private boolean reseted; + @Before public void setUp() { data = new byte[] { 'x', 'y', 'z' }; @@ -49,6 +53,16 @@ public void setUp() { public void close() { closed = true; } + + @Override + public void mark(int readAheadLimit) { + marked = true; + } + + @Override + public synchronized void reset() { + reseted = true; + } }; stream = new AutoCloseInputStream(targetStream); closed = false; @@ -105,19 +119,20 @@ public void testReadBufferOffsetLength() throws IOException { } @Test - public void testMark() throws IOException - { + public void testMark() { assertTrue(targetStream.markSupported()); // Make sure mark is disabled assertFalse(stream.markSupported()); // Check that mark() does not fail stream.mark(1); + assertFalse(marked); // Check that reset() throw an exception try { stream.reset(); } catch (IOException expected) { assertEquals("mark/reset not supported", expected.getMessage()); + assertFalse(reseted); } } From f6d18f1a24d6fb1838c951af1f4e132f7d73815d Mon Sep 17 00:00:00 2001 From: Thomas Mortagne Date: Mon, 19 Feb 2018 13:05:46 +0100 Subject: [PATCH 5/5] IO-568: AutoCloseInputStream sometimes breaks mark/reset contract --- .../io/input/AutoCloseInputStream.java | 61 ++++---- .../io/input/AutoCloseInputStreamTest.java | 139 ------------------ .../MarkEnabledAutoCloseInputStreamTest.java | 54 +++++++ 3 files changed, 86 insertions(+), 168 deletions(-) delete mode 100644 src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java create mode 100644 src/test/java/org/apache/commons/io/input/MarkEnabledAutoCloseInputStreamTest.java diff --git a/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java b/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java index 22eb8e04b4f..ba5aa04b609 100644 --- a/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java +++ b/src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java @@ -33,21 +33,35 @@ * closing the stream when no longer needed) or the underlying stream (by not * releasing resources once the last byte has been read) do not do that. *

- * Since the only way to always close the stream when reaching the end and - * respecting mark/reset contract, AutoCloseInputStream force disabling - * mark/reset. + * Since there is no safe way to always close the stream when reaching the end and + * respecting mark/reset contract, it's possible to force disabled mark support. + * This is highly recommended when you don't know how the stream is going to be used + * (mark is not disabled by default for retro compatibility reasons). * * @since 1.4 */ public class AutoCloseInputStream extends ProxyInputStream { + private boolean markEnabled = true; + /** * Creates an automatically closing proxy for the given input stream. * * @param in underlying input stream */ public AutoCloseInputStream(final InputStream in) { + this(in, true); + } + + /** + * Creates an automatically closing proxy for the given input stream. + * + * @param in underlying input stream + */ + public AutoCloseInputStream(final InputStream in, boolean markEnabled) { super(in); + + this.markEnabled = markEnabled; } /** @@ -83,42 +97,31 @@ protected void afterRead(final int n) throws IOException { } } - /** - * {@inheritDoc} - *

- * AutoCloseInputStream does not support mark/reset no matter what. - * - * @see org.apache.commons.io.input.ProxyInputStream#mark(int) - */ @Override public synchronized void mark(int readlimit) { - // Behave as standard InputStream not supporting mark/reset + if (this.markEnabled) { + super.mark(readlimit); + } } - /** - * {@inheritDoc} - *

- * AutoCloseInputStream does not support mark/reset no matter what. - * - * @see org.apache.commons.io.input.ProxyInputStream#reset() - */ @Override public synchronized void reset() throws IOException { - // Behave as standard InputStream not supporting mark/reset - throw new IOException("mark/reset not supported"); + if (this.markEnabled) { + super.reset(); + } else { + // Behave as standard InputStream not supporting mark/reset + throw new IOException("mark/reset not supported"); + } } - /** - * {@inheritDoc} - *

- * AutoCloseInputStream does not support mark/reset no matter what. - * - * @see org.apache.commons.io.input.ProxyInputStream#markSupported() - */ @Override public boolean markSupported() { - // Behave as standard InputStream not supporting mark/reset - return false; + if (this.markEnabled) { + return super.markSupported(); + } else { + // Behave as standard InputStream not supporting mark/reset + return false; + } } /** diff --git a/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java b/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java deleted file mode 100644 index 47b93527059..00000000000 --- a/src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java +++ /dev/null @@ -1,139 +0,0 @@ -/* - * Licensed to the Apache Software Foundation (ASF) under one or more - * contributor license agreements. See the NOTICE file distributed with - * this work for additional information regarding copyright ownership. - * The ASF licenses this file to You under the Apache License, Version 2.0 - * (the "License"); you may not use this file except in compliance with - * the License. You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.apache.commons.io.input; - -import java.io.ByteArrayInputStream; -import java.io.IOException; -import java.io.InputStream; - -import org.apache.commons.io.serialization.MockSerializedClass; -import org.junit.Before; -import org.junit.Test; - -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertTrue; - -/** - * JUnit Test Case for {@link AutoCloseInputStream}. - */ -public class AutoCloseInputStreamTest { - - private byte[] data; - - private InputStream targetStream; - - private InputStream stream; - - private boolean closed; - - private boolean marked; - - private boolean reseted; - - @Before - public void setUp() { - data = new byte[] { 'x', 'y', 'z' }; - targetStream = new ByteArrayInputStream(data) { - @Override - public void close() { - closed = true; - } - - @Override - public void mark(int readAheadLimit) { - marked = true; - } - - @Override - public synchronized void reset() { - reseted = true; - } - }; - stream = new AutoCloseInputStream(targetStream); - closed = false; - } - - @Test - public void testClose() throws IOException { - stream.close(); - assertTrue("closed", closed); - assertEquals("read()", -1, stream.read()); - } - - - @Test - public void testRead() throws IOException { - for (final byte element : data) { - assertEquals("read()", element, stream.read()); - assertFalse("closed", closed); - } - assertEquals("read()", -1, stream.read()); - assertTrue("closed", closed); - } - - @Test - public void testReadBuffer() throws IOException { - final byte[] b = new byte[data.length * 2]; - int total = 0; - for (int n = 0; n != -1; n = stream.read(b)) { - assertFalse("closed", closed); - for (int i = 0; i < n; i++) { - assertEquals("read(b)", data[total + i], b[i]); - } - total += n; - } - assertEquals("read(b)", data.length, total); - assertTrue("closed", closed); - assertEquals("read(b)", -1, stream.read(b)); - } - - @Test - public void testReadBufferOffsetLength() throws IOException { - final byte[] b = new byte[data.length * 2]; - int total = 0; - for (int n = 0; n != -1; n = stream.read(b, total, b.length - total)) { - assertFalse("closed", closed); - total += n; - } - assertEquals("read(b, off, len)", data.length, total); - for (int i = 0; i < data.length; i++) { - assertEquals("read(b, off, len)", data[i], b[i]); - } - assertTrue("closed", closed); - assertEquals("read(b, off, len)", -1, stream.read(b, 0, b.length)); - } - - @Test - public void testMark() { - assertTrue(targetStream.markSupported()); - - // Make sure mark is disabled - assertFalse(stream.markSupported()); - // Check that mark() does not fail - stream.mark(1); - assertFalse(marked); - // Check that reset() throw an exception - try { - stream.reset(); - } catch (IOException expected) { - assertEquals("mark/reset not supported", expected.getMessage()); - assertFalse(reseted); - } - } - -} diff --git a/src/test/java/org/apache/commons/io/input/MarkEnabledAutoCloseInputStreamTest.java b/src/test/java/org/apache/commons/io/input/MarkEnabledAutoCloseInputStreamTest.java new file mode 100644 index 00000000000..3e31ea65a23 --- /dev/null +++ b/src/test/java/org/apache/commons/io/input/MarkEnabledAutoCloseInputStreamTest.java @@ -0,0 +1,54 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.commons.io.input; + +import java.io.IOException; + +import org.junit.Test; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; + +/** + * JUnit Test Case for {@link AutoCloseInputStream} with enabled mark support (the default). + */ +public class MarkEnabledAutoCloseInputStreamTest extends AbstractAutoCloseInputStreamTest { + + public MarkEnabledAutoCloseInputStreamTest() { + super(true); + } + + @Test + public void testMark() throws IOException { + assertTrue(targetStream.markSupported()); + + // Make sure mark is disabled + assertTrue(stream.markSupported()); + + // Check that mark() does not fail + stream.mark(1); + assertTrue("not marked", marked); + + assertEquals('x', this.stream.read()); + + // Check that reset() works + stream.reset(); + assertTrue("not reseted", reseted); + + assertEquals('x', this.stream.read()); + } +}