Resource leak caused by CssImportPreProcessor

[GoogleCodeExporter]

What steps will reproduce the problem?
Use a css stylesheet that imports other resources such as background images.

What is the expected output? What do you see instead?
A resource leak occurs causing re-deployment and eclipse failures.

What version of the product are you using?
Wro4j 1.4.8.1
The old version 1.4.3 does not show the problem.

Please provide any additional information below.

The class CssImportPreProcessor does not close input streams created at 
ro.isdc.wro.model.resource.processor.impl.css.CssImportPreProcessor.getImportedR
esources(CssImportPreProcessor.java:159)

  private List<Resource> getImportedResources(final Resource resource)
    throws IOException {
    ...
    try {
      css = IOUtils.toString(uriLocatorFactory.locate(resource.getUri()), ...);
    } catch (IOException e) {
      ...
    }

Solution:

  private List<Resource> getImportedResources(final Resource resource)
    throws IOException {
    ...
    try {
      InputStream is = uriLocatorFactory.locate(resource.getUri());
      try {
        css = IOUtils.toString(is, ...);
      } finally {
        is.close();
      }
    } catch (IOException e) {
      ...
    }

Further possible leaks can be found via references of 
UriLocatorFactory.locate() ... e.g.

DataUriGenerator.generateDataURI(),
PreProcessorExecutor.getResourceContent(), 
PreProcessorExecutor.getResourceContent() and so on.

  private String getResourceContent(final Resource resource) {
    ...
    final InputStream is = ...
    final String result = IOUtils.toString(is, ...);
    is.close();
    ...

... can be solved using "try { ... } finally { is.close() }" pattern.

    final InputStream is = ...
    final String result;
    try {
      result = IOUtils.toString(is, ...);
    } finally {
      is.close();
    }

Original issue reported on code.google.com by g...@inasys.de on 16 Aug 2012 at 1:33

[GoogleCodeExporter]

Is this a critical bug? Do you think it requires a quick release? Or it is ok 
to release it later as part of 1.4.9?

Original comment by alex.obj...@gmail.com on 16 Aug 2012 at 1:35

• Changed state: Accepted
• Added labels: Milestone-Release-1.4.9

[GoogleCodeExporter]

It is not critical. As a workaround - to get rid all possible leaks - we 
override BaseWroManagerFactory

  @Override
  protected UriLocatorFactory newUriLocatorFactory() {
    // workaround to prevent file handle leaks
    return new SimpleUriLocatorFactory() {
       { // anonymous c'tor
        addUriLocator(new ServletContextUriLocator() {
          public InputStream locate(final String uri) throws IOException {
            return MyWroManagerFactory.copyStream(super.locate(uri));
          }          
        });

        addUriLocator(new ClasspathUriLocator() {
          public InputStream locate(final String uri) throws IOException {
            return MyWroManagerFactory.copyStream(super.locate(uri));
          }
        });

        addUriLocator(new UrlUriLocator() {
          public InputStream locate(final String uri) throws IOException {
            return MyWroManagerFactory.copyStream(super.locate(uri));
          }          
        });

      }
    };
  }  

  static InputStream copyStream(InputStream is) throws IOException {
    try {
      return org.apache.commons.io.output.ByteArrayOutputStream.toBufferedInputStream(is);
    } finally {
      is.close();
    }
  }

Original comment by g...@inasys.de on 16 Aug 2012 at 1:43

[GoogleCodeExporter]

Have you considered using AutoCloseInputStream from commons-io instead of 
copyStream method?

Original comment by alex.obj...@gmail.com on 16 Aug 2012 at 2:52

[GoogleCodeExporter]

Fixed in branch 1.4.x (using AutoCloseInputStream everywhere).

Could you please try to build it locally and let me know if you have any 
problems?
Thanks.

Original comment by alex.obj...@gmail.com on 16 Aug 2012 at 3:16

• Changed state: Fixed

[GoogleCodeExporter]

Does AutoCloseInputStream close the underlying stream in case of exceptions?
What happens if an exception occurs and the end of input stream has not been 
reached?

OK, the class has a finalize method, but you should not rely on.

Original comment by g...@inasys.de on 16 Aug 2012 at 3:29

[GoogleCodeExporter]

So you would still recommend using try {} finally approach instead of relying 
on AutoCloseStream? 

If default implementation of AutoCloseStream doesn't handle exceptions, I think 
we could extend AutoCloseStream and close the stream when exception is thrown. 
This would be much cleaner than using try {} finally....

Original comment by alex.obj...@gmail.com on 16 Aug 2012 at 3:37

[GoogleCodeExporter]

Consider the following situation:

  InputStream is = ...
  doSomething(is);
  is.close();

If method doSomething() throws an exception the input stream is leaked. 

If you decorate the input stream by AutoCloseInputStream

  InputStream is = new AutoCloseInputStream(...);
  doSomething(is);
  is.close();

the situation remains the same. If method doSomething() throws an exception 
somewhere in the middle of the stream the underlying input stream remains open.

Original comment by g...@inasys.de on 16 Aug 2012 at 3:44

[GoogleCodeExporter]

I understand this scenario. However, it is possible to create a custom 
AutoCloseInputStream and override the 

protected void handleIOException(IOException e) throws IOException 

method which just throws exception by default. The custom implementation would 
force close of the decorated inputStream. 

Original comment by alex.obj...@gmail.com on 16 Aug 2012 at 3:48

[GoogleCodeExporter]

Reopen in order to find a better approach (extending AutoCloseInputStream or 
something else).

Original comment by alex.obj...@gmail.com on 16 Aug 2012 at 3:51

• Changed state: Accepted

[GoogleCodeExporter]

I understand your idea. But the method handleIOException() could only handle 
exceptions that are caused by the stream itself. 

What happens if method doSomething() does the following:

private doSomething(InputStream is) {
  int b = is.read(); // read first byte of total 1234567 bytes.
  if (b != 42) {
    throw new IllegalStateException("That's not the correct answer.");
  }
}

Original comment by g...@inasys.de on 16 Aug 2012 at 3:57

[GoogleCodeExporter]

I agree with this. 
But then comes the question: wouldn't it be the responsibility of such code to 
close the stream? As a result, any leaks would indicate a problematic code 
which should be fixed. What do you think?

Original comment by alex.obj...@gmail.com on 16 Aug 2012 at 4:00

[GoogleCodeExporter]

The example above throws an exception explicitly. But the method could cause 
any implicit failure resulting in NullPointerExceptions, TimeoutException and 
so on.

In my opinion, the best solution is the "try {} finally pattern" (for Java 6 
and Java 5):

  InputStream is = ...
  try {
    doSomething(is);
  } finally {
    is.close();
  }

Original comment by g...@inasys.de on 16 Aug 2012 at 4:32

[GoogleCodeExporter]

Ok, will do.

Thanks.

Original comment by alex.obj...@gmail.com on 16 Aug 2012 at 4:45

[GoogleCodeExporter]

Original comment by alex.obj...@gmail.com on 16 Aug 2012 at 6:08

• Changed state: Fixed