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

Update IOUtils.java #7620

Closed
wants to merge 2 commits into from
Closed

Update IOUtils.java #7620

wants to merge 2 commits into from

Conversation

puneetbehl
Copy link
Contributor

Refactor code around file system to synchronously access file system when using JAR.

Fixes grails/grails-core#12589

Refactor code around file system to synchronously access file system when using JAR.

Fixes grails/grails-core#12589
@puneetbehl puneetbehl requested a review from graemerocher June 28, 2022 05:11
@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

53.6% 53.6% Coverage
0.0% 0.0% Duplication

@puneetbehl puneetbehl requested a review from sdelamo June 28, 2022 13:19
@yawkat
Copy link
Member

yawkat commented Jun 28, 2022

If the file system is already created (FileSystemAlreadyExistsException above) because another thread also has it open, then closing it could break whatever operation that other thread is doing. This should not happen if the only thing accessing the URI is IOUtils, because of the synchronized (IOUtils.class), but it could happen if another class is accessing the same file. Otherwise, the handling of FileSystemAlreadyExistsException would be pointless.

Since this is about jar files, i.e. zipfs, I think it would be a better approach to use FileSystems.newFileSystem(Path), instead of the URI-based factory. The Path factory does not produce the FileSystemAlreadyExistsException, it always creates a new file system. This would also remove the need for synchronization.

@graemerocher
Copy link
Contributor

Needs to be tested to ensure nothing breaks in native

@puneetbehl
Copy link
Contributor Author

I tried to run some basic smoke test for Micronaut graalvm native images and it seems to run fine. I will also try the my first graalvm app guide (https://guides.micronaut.io/latest/micronaut-creating-first-graal-app.html) today and share my findings here.

@sdelamo sdelamo added this to the 3.5.3 milestone Jul 7, 2022
yawkat added a commit that referenced this pull request Jul 7, 2022
This patch moves IOUtils zip handling to use `FileSystems.newFileSystem(Path, ...)` API instead of the URI-based method. The URI-based FS is shared globally, so it can lead to race conditions when opening/closing file systems. The Path-based FS is unshared and can be used safely.

Additionally, this patch implements support for nested jar files. For Java 11+ this works with default zipfs, but earlier versions do not support zipfs nesting, so there's a fallback to extract the intermediate jar instead.

Resolves #7620
Resolves #7626
sdelamo pushed a commit that referenced this pull request Jul 11, 2022
This patch moves IOUtils zip handling to use `FileSystems.newFileSystem(Path, ...)` API instead of the URI-based method. The URI-based FS is shared globally, so it can lead to race conditions when opening/closing file systems. The Path-based FS is unshared and can be used safely.

Additionally, this patch implements support for nested jar files. For Java 11+ this works with default zipfs, but earlier versions do not support zipfs nesting, so there's a fallback to extract the intermediate jar instead.

Resolves #7620
Resolves #7626
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.

Runnable jar fails for 5.2.0
4 participants