Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IO-568: AutoCloseInputStream should disable mark/reset #55

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@
* 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.
* <p>
* 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
*/
Expand Down Expand Up @@ -79,6 +83,44 @@ protected void afterRead(final int n) throws IOException {
}
}

/**
* {@inheritDoc}
* <p>
* 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
}

/**
* {@inheritDoc}
* <p>
* 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");
}

/**
* {@inheritDoc}
* <p>
* 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;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tmortagne

Again, this completely changes the contract of the existing class and will break existing call sites. I think there are two kinds of solutions here:
(1) Write a new class as I suggested before.
(2) Implement mark/reset with its own buffer so that reset works even when the underlying stream is actually closed. I'm not even sure of the sanity of that.

Gary

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't understand this "again". The whole point of the previous conversation was that it was simpler to make AutoCloseInputStream just cancel mark/reset support since it's too complex to really support it properly and you said yourself

It seems to me like the simplest solution would be for markSupported() to return false.

As I said AutoCloseInputStream does not make much sense in its current state since it break InputStream contract regarding mark/reset support which is not so great. Cancel mark/rest support is one way to prevent this inconsistency which is what I suggested several times and that you seemed to agree on.

If you are suggesting that a new class should be introduced and AutoCloseInputStream deprecated because broken but not fixable it was not very clear.

/**
* 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,37 +16,55 @@
*/
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;

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' };
stream = new AutoCloseInputStream(new ByteArrayInputStream(data) {
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;
}

Expand Down Expand Up @@ -100,4 +118,22 @@ public void testReadBufferOffsetLength() throws IOException {
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);
}
}

}