Fix failing to delete temporary remapping files #935
Closed
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
This exact method is called twice later in this same function and always with a try-with-resources. Update this occurrence to match rather than use an explicit close call.
JarURLConnection contains internal caching of opened JARs. This causes the JVM to maintain a handle to any JAR file accessed this way. Said handle remains open until the cache invalidates or the JVM terminates. In the case of a short-lived program such as Fabric Loader, it is usually JVM termination. This prevents us from deleting the temporary remapping files after reading it's manifest. Since this cache is theoretically useful in the context of Log4jLogHandler, leave it up to the caller to decide whether or not it should be bypassed.
|
As discussed in the issue, I believe it might be a good idea to simply remove (or empty) the There is no attempt made to reuse these temporary files, nor do I think there should be. However this means if deletion ever fails for whatever reason (e.g. process terminated early) the files are now permanently there unless manually deleted by the user. The directory can be safely removed whenever remapping begins to ensure any stray temporary files from previous executions are deleted. |
|
Superseeded by: #936 thanks for the PR. |
# for free
to join this conversation on GitHub.
Already have an account?
# 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.
Fixes #920
Not sure how to unit test this but I am open to suggestions