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.

Sign up for GitHub

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

Close JarFile #1970

Merged
merged 4 commits into from
Jan 5, 2021
Merged

Close JarFile #1970

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fb57629
Close JarFile
laurit Dec 30, 2020
c1c4b22
test loading exporter jar that has a space in the path
laurit Jan 4, 2021
4f2251b
change class to private
laurit Jan 4, 2021
a58a2aa
try to convince groovy compiler not to create references to java.lang…
laurit Jan 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/ExporterClassLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import java.io.IOException;
import java.io.InputStream;
import java.net.URISyntaxException;
import java.net.URL;
import java.net.URLClassLoader;
import java.util.Enumeration;
Expand Down Expand Up @@ -127,10 +128,9 @@ private static String getPackageName(String className) {
}

private static Manifest getManifest(URL url) {
try {
JarFile jarFile = new JarFile(url.getFile());
try (JarFile jarFile = new JarFile(url.toURI().getPath())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to change the url.getFile()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If path contains a space then url.getFile() will leave it as %20, url.toURI().getPath() decodes it as an actual space character

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it cause a problem? I'm surprised the more direct way of getting a File would cause JarFile constructor to fail, if you're seeing an exception let's add a comment above this line on what exception we are trying to prevent with the indirect code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

URL.getFile returns a String. As the url is constructed with new File(exporterJar).toURI().toURL(); which encodes space -> %20 when getting file from url we should use a way that translates %20 back to space.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it sounds like there is a problem when the path has a space. If so yeah we need to fix it :) But it's confusing, so we need at least a comment documenting the failure case. Even better is a test that fails before the change and passes after, then no one will accidentally revert and regress.

Alternatively, we can keep the code similarly as before and file an issue - the PR title is about closing the jar file but is doing more than just that. It's ok but we need to do it completely, otherwise let's not make too many changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added test

return jarFile.getManifest();
} catch (IOException e) {
} catch (IOException | URISyntaxException e) {
log.warn(e.getMessage(), e);
}
return null;
Expand Down