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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7c042b8
IO-568: AutoCloseInputStream crash on reset() when reading the whole …
tmortagne Feb 7, 2018
56d2264
IO-568: AutoCloseInputStream should disable mark/reset
tmortagne Feb 8, 2018
508c6ab
IO-568: AutoCloseInputStream should disable mark/reset
tmortagne Feb 9, 2018
2b1cdb3
IO-568: AutoCloseInputStream should disable mark/reset
tmortagne Feb 9, 2018
f6d18f1
IO-568: AutoCloseInputStream sometimes breaks mark/reset contract
tmortagne Feb 19, 2018
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
38 changes: 34 additions & 4 deletions src/main/java/org/apache/commons/io/input/AutoCloseInputStream.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -37,6 +37,8 @@
*/
public class AutoCloseInputStream extends ProxyInputStream {

private boolean marked;

/**
* Creates an automatically closing proxy for the given input stream.
*
Expand Down Expand Up @@ -66,19 +68,47 @@ 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
* @since 2.0
*/
@Override
protected void afterRead(final int n) throws IOException {
if (n == EOF) {
if (n == EOF && !marked) {
Copy link
Member

Choose a reason for hiding this comment

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

This changes the contract of the method and the whole class. The class Javadoc states "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." If feels like you need a different class. Now, it is possible that the original author did not even think of the mark and reset use cases, but still, this has the potential of defeating the purpose of the class: which is to auto close no matter what. Maybe you need a class called "AutoCloseUnmarkedInputStream"?

Copy link
Member Author

@tmortagne tmortagne Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

The thing is AutoCloseInputStream itself does not respect the contract of mark/reset API (which makes Tika confuse) and I feel this use case was simply forgotten. Before anything, it's supposed to act as a valid InputStream (which happen to be automatically closed when you don't need it anymore). I assumed that if you mark it it means you will reset it at some point so there is no much risk IMO.

IMO either AutoCloseInputStream support mark/rest or it should force override mark/reset and return false for markSupported() (which is fine with me too since it fixes my use case, I just assumed a more subtle solution would be preferred :)).

Copy link
Member

@garydgregory garydgregory Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I do see your point and let me rephrase it to make sure I understand: Either we should implement the mark/reset APIs properly or we should implement markSupported() to return false.

My concern remains for this use case, today:

  • I open a stream
  • I read
  • I call mark() and read() here and there
  • I read to the end of stream and the stream closes as it should
  • I do not bother to call close()
  • All is well

With this change, my random calls to mark() before I read to the end of the stream cause the stream to remain open. Clearly a leak. Granted, I should close a stream that I open somewhere or in a try-with-resources block but this class lives outside of this use case.

It seems to me like the simplest solution would be for markSupported() to return false. WDYT? Or create a new class.

Copy link
Member Author

Choose a reason for hiding this comment

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

My main point is that it cannot stay like it currently is.

So yes if you want to be sure mark() can be a random mistake then mark/reset should throw an exception and markSupported() should always return false.

I can change my pull request in that directly, no problem. Both works.

close();
}
}

/**
* {@inheritDoc}
* <p>
* 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}
* <p>
* Reset the marked flag.
*
* @see org.apache.commons.io.input.ProxyInputStream#reset()
*/
@Override
public synchronized void reset() throws IOException {
super.reset();

marked = 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
44 changes: 40 additions & 4 deletions src/test/java/org/apache/commons/io/input/AutoCloseInputStreamTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,17 @@
*/
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.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}.
*/
Expand Down Expand Up @@ -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);
}

}