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

Classpath instrumentation of NIO.2 operations fails for paths in zip archives #25044

Closed
netvl opened this issue May 11, 2023 · 6 comments · Fixed by #26117
Closed

Classpath instrumentation of NIO.2 operations fails for paths in zip archives #25044

netvl opened this issue May 11, 2023 · 6 comments · Fixed by #26117
Labels
a:bug good first issue Good for newcomers 🌳 help wanted Taking contributor PRs, might need existing Gradle knowledge in:configuration-cache Configuration Caching
Milestone

Comments

@netvl
Copy link

netvl commented May 11, 2023

Expected Behavior

Using Files.readString API in plugin code when reading non-default file systems should work correctly.

Current Behavior

When some code which runs within Gradle (a custom plugin, in most cases) tries to execute Files.readString without specifying the charset against a Path instance created from a zip file system, this method call will fail due to incorrect instrumentation within Gradle:

java.lang.UnsupportedOperationException
	at jdk.zipfs/jdk.nio.zipfs.ZipPath.toFile(ZipPath.java:674)
	at org.gradle.internal.classpath.Instrumented.filesReadString(Instrumented.java:419)
	at org.gradle.internal.classpath.declarations.NioFileInterceptors.intercept_readString(NioFileInterceptors.java:169)
	...

The reason, as it seems, is that the path is attempted to be converted into a File instance unconditionally:

listener().fileOpened(file.toFile(), consumer);

This naturally fails for non-local file system paths.
It might be that some other similarly instrumented operations are affected as well.

Context (optional)

Files.readString and other similar operations are very common in plugins, so it would make sense for it to work without hitches. It seems that this error started to happen relatively recently, probably because Gradle didn't do this kind of instrumentation earlier.

Steps to Reproduce

Any way to execute Files.readString() against a Path inside a zip archive while within the instrumented Gradle classpath should demonstrate the issue. In our case, this method was called from within our internal plugin which uses zip file system to read zip archive contents.

Gradle version

8.1.1

Build scan URL (optional)

No response

Your Environment (optional)

No response

@jbartok
Copy link
Member

jbartok commented May 19, 2023

Sorry that you're having trouble with Gradle!

We appreciate the effort that went into filing this issue, but we must ask for more information.

As stated in our issue template, a minimal reproducible example is a must for us to be able to track down and fix your problem efficiently. Our available resources are severely limited, and we must be sure we are looking at the exact problem you are facing.

If we have a reproducer, we may be able also to suggest workarounds or ways to avoid the problem.

The ideal way to provide a reproducer is to leverage our reproducer template. You can also use Gradle Project Replicator to reproduce the structure of your project.

This issue will be closed after 7 days unless you provide more information.

@jbartok jbartok added the pending:reproducer Indicates that the issue requires a reproducer or will be closed after 7 days label May 19, 2023
@netvl
Copy link
Author

netvl commented May 24, 2023

@jbartok please find the reproducer here: https://github.com/netvl/gradle-classpath-instrumentation-bug
See its github action results for the failure example.

@github-actions github-actions bot removed the pending:reproducer Indicates that the issue requires a reproducer or will be closed after 7 days label May 24, 2023
@jbartok jbartok added in:configuration-cache Configuration Caching and removed to-triage labels May 26, 2023
@jbartok
Copy link
Member

jbartok commented May 26, 2023

Thank you for providing a valid reproducer.

The issue is in the backlog of the relevant team and is prioritized by them.

noeppi-noeppi added a commit to ModdingX/ModGradle that referenced this issue Jun 8, 2023
Matyrobbrt added a commit to Matyrobbrt/RegistrationUtils that referenced this issue Jun 13, 2023
@netvl
Copy link
Author

netvl commented Jul 4, 2023

Is there any chance of this being fixed sooner? Looking at the code and comparing two adjacent methods:

public static String filesReadString(Path file, String consumer) throws Throwable {
listener().fileOpened(file.toFile(), consumer);
return (String) FILES_READ_STRING_PATH.get().invokeExact(file);
}

public static String filesReadString(Path file, Charset charset, String consumer) throws Throwable {
FileUtils.tryReportFileOpened(file, consumer);
return (String) FILES_READ_STRING_PATH_CHARSET.get().invokeExact(file, charset);
}

it looks like the fix will be a very simple one-liner, since the readString(Path, Charset) instrumented body looks correct.

@mlopatkin mlopatkin added 🌳 help wanted Taking contributor PRs, might need existing Gradle knowledge good first issue Good for newcomers labels Aug 8, 2023
@mlopatkin
Copy link
Member

mlopatkin commented Aug 8, 2023

This issue is a good choice for first-time contributors to Gradle, it is actionable and ready for contribution.

See CONTRIBUTING.md for more information.

@wuyangnju
Copy link
Contributor

Hi @mlopatkin, this is my first PR to Gradle, could you help me assign a reviewer? #26117

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:bug good first issue Good for newcomers 🌳 help wanted Taking contributor PRs, might need existing Gradle knowledge in:configuration-cache Configuration Caching
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants