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

Double close causes exception, which violates Closeable contract. #16

Merged
merged 2 commits into from
Jul 12, 2013

Conversation

stevenschlansker
Copy link
Contributor

LZ4's block streams inherit Closeable. Closeable's contract specifies:

    /**
     * Closes this stream and releases any system resources associated
     * with it. If the stream is already closed then invoking this
     * method has no effect.
     *
     * @throws IOException if an I/O error occurs
     */
    public void close() throws IOException;

Show that double-close is broken in a way that violates the contract. Add a fix.

Steven Schlansker added 2 commits July 3, 2013 13:47
    /**
     * Closes this stream and releases any system resources associated
     * with it. If the stream is already closed then invoking this
     * method has no effect.
     *
     * @throws IOException if an I/O error occurs
     */
    public void close() throws IOException;

Show that double-close is broken in a way that violates the contract.
@stevenschlansker
Copy link
Contributor Author

OK, in practice I think this is not a big deal (a stream which is finished but not closed seems useless to me) but I have updated the PR. Does it look good now?

jpountz added a commit that referenced this pull request Jul 12, 2013
Double close causes exception, which violates Closeable contract.
@jpountz jpountz merged commit 7ff7fa0 into lz4:master Jul 12, 2013
@jpountz
Copy link
Collaborator

jpountz commented Jul 12, 2013

The use-case for not closing a finished stream is to be able to keep writing data on the underlying stream. However I agree that most users won't use this feature and just close the stream when they are done adding data to it.

The PR looked good, I just merged it. Thanks!

@stevenschlansker stevenschlansker deleted the double-close branch July 15, 2013 23:47
@stevenschlansker
Copy link
Contributor Author

Hi @jpountz, can you make a release with this fix? Thanks!

@jpountz
Copy link
Collaborator

jpountz commented Aug 6, 2013

There are a couple of issues that I would like to fix before the next release, I'll do it ASAP.

@jpountz
Copy link
Collaborator

jpountz commented Aug 8, 2013

@stevenschlansker 1.2.0 should be available from the maven repo now, it contains your fix.

@stevenschlansker
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants