Resources: closing outputstream bug, recursive delete #1176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NielsCharlier
force-pushed
the
filesystem
branch
from
5860960
to
f1aa35e
Compare
NielsCharlier
force-pushed
the
filesystem
branch
from
f1aa35e
to
abc11ac
Compare
| } | ||
| finally { | ||
| lock.release(); | ||
| //if already closed, there should be no exception (see spec Closeable) |
There was a problem hiding this comment.
That is tricky logic, would love to know the file was written out at lest once :) Thinking about a resource against a directory with no write access. The tmp file would be created, the move would fail, and each successive close would produce the same error ...
You can decide if you care.
|
Output stream close looks fine. The Resource.delete() change requires further discussion/work. |
|
Revised and applied to master and 2.7.x / 2.6.x. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to the Closeable API (http://docs.oracle.com/javase/7/docs/api/java/io/Closeable.html#close%28%29 ) the close method should not throw (or cause, in this case) an exception when called on an already closed resource. The FileSystemResource and ResourceWrapper do do this.
Furthermore, I propose support for recursively deleting directories with their contents. This is already the case for the store.remove() function, but not resource.delete(). Note that the rule on whether to return true/false when the resource doesn't exist is different for remove and delete. Only for remove this appears to be documented.