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

Workaround for classpath resource concurrent access issue #37

Closed
wants to merge 1 commit into from

Conversation

psiroky
Copy link

@psiroky psiroky commented Oct 6, 2017

getResourceAsStream() is not equivalent to getResource() + openStream().
In concurrent env. the former sometimes returns and already closed
stream.

This is a workaround for the following issue we are encountering when using Freemarker for annotation processing in parallel maven build (-T). This is unfortunately just a workaround and such is not ideal, so fell free to reject this.

The stacktrace of the error:

18:44:24 [ERROR] /home/jenkins/workspace/guvnor/upstream-repos/uberfire/uberfire-showcase/uberfire-client-webapp/src/main/java/org/uberfire/client/screens/DemoSplashScreen.java:[34,1] Internal error in org.uberfire.annotations.processors.WorkbenchSplashScreenProcessorjava.lang.ExceptionInInitializerError
18:44:24 [ERROR] at org.uberfire.annotations.processors.AbstractGenerator.<init>(AbstractGenerator.java:44)
18:44:24 [ERROR] at org.uberfire.annotations.processors.SplashScreenActivityGenerator.<init>(SplashScreenActivityGenerator.java:41)
18:44:24 [ERROR] at org.uberfire.annotations.processors.WorkbenchSplashScreenProcessor.<init>(WorkbenchSplashScreenProcessor.java:49)
18:44:24 [ERROR] at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
....
18:44:24 [ERROR] at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
18:44:24 [ERROR] at java.lang.Thread.run(Thread.java:745)
18:44:24 [ERROR] Caused by: java.lang.IllegalStateException: zip file closed
18:44:24 [ERROR] at java.util.zip.ZipFile.ensureOpen(ZipFile.java:669)
18:44:24 [ERROR] at java.util.zip.ZipFile.getEntry(ZipFile.java:309)
18:44:24 [ERROR] at java.util.jar.JarFile.getEntry(JarFile.java:240)
18:44:24 [ERROR] at sun.net.www.protocol.jar.URLJarFile.getEntry(URLJarFile.java:128)
18:44:24 [ERROR] at sun.net.www.protocol.jar.JarURLConnection.connect(JarURLConnection.java:132)
18:44:24 [ERROR] at sun.net.www.protocol.jar.JarURLConnection.getInputStream(JarURLConnection.java:150)
18:44:24 [ERROR] at java.net.URLClassLoader.getResourceAsStream(URLClassLoader.java:238)
18:44:24 [ERROR] at freemarker.template.Configuration.<clinit>(Configuration.java:431)

After applying the workaround we never saw that exception again (with several hundreds of builds).

getResourceAsStream() is not equivalent to getResource() + openStream().
In concurrent env. the former sometimes returns and already closed
stream.
@ddekany
Copy link
Contributor

ddekany commented Oct 6, 2017

I'm sure that getResourceAsStream is supposed to be thread safe. If closing matters, it should always return a new instance. Is this a bug in the ClassLoader that Maven uses? I have used getResourceAsStream for many times in my life, and now I'm scared. (:

@psiroky
Copy link
Author

psiroky commented Oct 6, 2017

@ddekany tbh, I don't know. It looks like an issue with the Classloader method, but I was not able to confirm that. I believe this is not an issue with the Freemarker itself.

I've spent some hours trying to find the root cause, but it's even very hard to reproduce. I haven't found a stable reproducer, only one which I need to run like 30 times and hope it fails :(

asfgit pushed a commit that referenced this pull request Oct 6, 2017
…s (caused by bugs outside of FreeMarker) when loading resources included in the FreeMarker jar (see freemarker.template.utility.ClassUtil.getReasourceAsStream). Related to #37.
@ddekany
Copy link
Contributor

ddekany commented Oct 6, 2017

I have committed a bit more generic and hopefully a bit safer (or rather more backward compatible) fix against this. See: bd7498e. It will go into 2.3.27. Can you confirm that it works? If so, please also close this issue. Thanks for reporting this and showing a workaround!

@psiroky
Copy link
Author

psiroky commented Oct 6, 2017

Thanks for the quick fix! I'll run our build and will report back.

@psiroky
Copy link
Author

psiroky commented Oct 7, 2017

The fix is unfortunately not working in 100% of the cases, please see my comment for more details: bd7498e#commitcomment-24827266

asfgit pushed a commit that referenced this pull request Oct 8, 2017
…properties. Related to #37.

Also, fixed bug where we didn't try to find TLD resources with the thread context class loader.
@ddekany
Copy link
Contributor

ddekany commented Oct 8, 2017

Ah, of course, it can also concurrently close the ZipFile while we are reading the InputStream... Hopefully I have worked that around too with this: 15b97af (Yeah, I go to too great lengths here to utilize any caching that exists behind getResourceAsStream... when it's not broken...)
Please report back and close if it works now!

@psiroky
Copy link
Author

psiroky commented Oct 9, 2017

I've executed ~250 builds with the latest fix (d2a2606) and all seems good, no build errors. Thanks a lot @ddekany!

@psiroky psiroky closed this Oct 9, 2017
@psiroky psiroky deleted the zip-file-closed branch October 9, 2017 12:10
@ddekany
Copy link
Contributor

ddekany commented Oct 9, 2017

What Java version are you using though? There's a few JDK bug fix that addresses similar (even if not identical) issues, like: https://bugs.openjdk.java.net/browse/JDK-6947916.

@psiroky
Copy link
Author

psiroky commented Oct 9, 2017

1.8.0_144. Looks like the linked issue is fixed only in 9+.

@psiroky
Copy link
Author

psiroky commented Oct 9, 2017

Ah, now I noticed the bug also mentions fix version 1.8.0_161. I could try with that version. If it works would you prefer to revert the workarounds?

@ddekany
Copy link
Contributor

ddekany commented Oct 10, 2017

I just realize that those 1.8 versions (152 and 161) weren't even released yet... The referred 1.7 was released 9 months ago. Anyway, for now I would keep the workaround, at least in the 2.3.x line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants