search

vaadin / charts

Framework 8 version of Vaadin Charts
Other
54 stars 84 forks source link

SVGGenerator doesn't properly close streams in the latest Charts 4.0.5 #541

Open FollowSteph opened 6 years ago

FollowSteph commented 6 years ago

If something bad happens on the server it can lead to the following exceptions:

Caused by: java.io.IOException: Stream closed
    at com.vaadin.addon.charts.util.SVGGenerator.generate(SVGGenerator.java:271)
    at com.vaadin.addon.charts.util.SVGGenerator.generate(SVGGenerator.java:218)

Once this exception happens it will happen to any further calls to SVGGenerator. In other words from that point on SVGGenerator will stop working.

Looking at the code it appears as though the streams are not correctly closed if there is an exception in writeTemporaryFileContents(). Specifically streams should be closed either as part of the try-with-resources or in a finally block however instead they are closed in the code. This is true for the above method and elsewhere in that class. The issue is there can be an exception and should there be an exception then it's possible that JS_STUFF has not been cleaned up and so when it then tries to run it again the if(!JS_STUFF.exists) will skip the creation of the temp files and then any code that relies on those streams will get the Stream closed exception. At least that's what I think, I'm not exactly sure as I don't know this code that well. The other part of the logs show the following stack trace:

Caused by: java.lang.RuntimeException: java.io.IOException: Stream closed
    at java.lang.ProcessBuilder$NullOutputStream.write(ProcessBuilder.java:433)
    at java.io.OutputStream.write(OutputStream.java:116)
    at java.io.BufferedOutputStream.flushBuffer(BufferedOutputStream.java:82)
    at java.io.BufferedOutputStream.flush(BufferedOutputStream.java:140)
    at com.vaadin.addon.charts.util.SVGGenerator.generate(SVGGenerator.java:252)

In essence the streams are not cleaned up nicely and the code seems to assume there can never be any exceptions. In either case I'm experiencing the following exceptions if there's an interruption in the server's thread.

alvarezguille commented 6 years ago

Thanks for reporting this, IIRC current behavior is that it does a single effort to create the files, but that's unfortunate as you say as it won't recover from an error when creating the files. We will take a look, unfortunately it's not so trivial to reproduce and have a proper regression test for this case.

FollowSteph commented 6 years ago

Would it not be possible to at the very lease just put the streams in a try/catch/finally block (or try-with-resources) block. I know this won't cover all the cases but at the very least it should eliminate some issues if there is an exception. Especially in the writeTemporaryFileContents() method. The close() methods on the streams need to be properly handled and shouldn't assume there won't ever be exceptions in that code, even if it's to temporary files.

And that being said if there is an exception, as a temporary solution (not ideal but should work for now), would it not be possible to just reset the process if there is an exception (in case some of the temporary files are completed but not all). That is call SVGGenerator.destroy() and then startPhantomJS(). The goal being that any temporary files are attached to the process so if it fails then a new process will be created (I only quickly looked at the code so I could be out to lunch on how it works). Yes it will leave some residue, etc. and it's very much less than ideal, but as a temporary stop gap it would prevent all SVGGenerators from having the this exception once it fails. Otherwise if it ever runs into an error state the SVGGenerator is down and will continue to throw exceptions until the server is bounced which is really bad for business as you can imagine.

Right now I've basically added in our code SVGGenerator.getInstance().destroy() if there is an exception to try to recover without a server restart but I'm not sure if it will work long term, because as you yourself said, it's hard to test.