Wicket
  1. Wicket
  2. WICKET-4293

UrlResourceStream closes incorrect InputStream causing stacktraces on undeploy on GlassFish

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.5.3
    • Fix Version/s: 1.5.4, 6.0.0-beta1
    • Component/s: wicket
    • Environment:

      Description

      The UrlResourceStream, used by PackageResources, uses URLConnection#getInputStream() to get file contents. This method is called in UrlResourceStream#getInputStream(), but also when closing the resource in UrlResourceStream#close(). At least on GlassFish v2 and v3, the second call to URLConnection#getInputStream() returns a new stream, so the one created to retrieve the file contents is never closed properly.
      This results in a warning of the container when the classes are garbage-collected, for example on undeploy.

      The problem is not triggered in all situations. It can be reproduced by using Wicket in a multi-module project consisting of an EAR, WAR and JAR. The JAR must contain a resource (CSS, image, ...) and a Behavior. Inside the Behavior, a static ResourceReference must be created for the resource file.
      When using this Behavior from inside the WAR project by loading a page the resource is loaded properly. On undeploy however, the described problem will show up.

      The problem does not exists in Wicket 1.4.x, because a reference to the InputStream is stored. A quickstart and patch for 1.5 is available, will try to attach it

      1. wicket-core-4293-2.patch
        6 kB
        Pepijn de Geus
      2. patch.txt
        2 kB
        Pepijn de Geus
      3. Wicket153-bug.zip
        22 kB
        Pepijn de Geus

        Issue Links

          Activity

          Hide
          Pepijn de Geus added a comment -

          Quickstart to reproduce the bug. Must be ran on GlassFish to be able to see the problem.

          The patch on Wicket 1.5-SNAPSHOT fixes the problem.

          Show
          Pepijn de Geus added a comment - Quickstart to reproduce the bug. Must be ran on GlassFish to be able to see the problem. The patch on Wicket 1.5-SNAPSHOT fixes the problem.
          Hide
          Martin Grigorov added a comment -

          We have test for this situation but it doesn't open a new input stream for each call: org.apache.wicket.util.resource.UrlResourceStreamTest.loadJustOnce()

          I just committed an improvement that should do what you need.
          Try it and give us feedback,
          Also try to break/improve the test if you can.
          Thanks!

          Show
          Martin Grigorov added a comment - We have test for this situation but it doesn't open a new input stream for each call: org.apache.wicket.util.resource.UrlResourceStreamTest.loadJustOnce() I just committed an improvement that should do what you need. Try it and give us feedback, Also try to break/improve the test if you can. Thanks!
          Hide
          Pepijn de Geus added a comment -

          Thanks for your reply, Martin.

          Your commit fixes the problem, but has one disadvantage: the call to Connections.close(data.connection) still opens a new InputStream and immediately closes it.
          The problem is thereby even broader: every call to Connections.close(URLConnection) has this problem. Please refer to these implementations of URLConnection (in OpenJDK and GlassFish) to see a new stream created for every call:

          http://grepcode.com/file/repository.springsource.com/org.apache.geronimo/com.springsource.org.apache.geronimo.kernel/2.0.1/org/apache/geronimo/kernel/classloader/JarFileUrlConnection.java?av=h#JarFileUrlConnection
          http://grepcode.com/file/repo1.maven.org/maven2/org.glassfish.common/common-util/10.0-b28/com/sun/enterprise/loader/EJBClassLoader.java?av=h#EJBClassLoader.InternalJarURLConnection

          In my opinion all usages of the close(URLConnection) with connections other than HttpURLConnection are faulty and should be checked/changed.
          I'll attach a new patch (wicket-core-4293-2.patch) fixing the problem based on your new commit and extending the test you mentioned.

          Show
          Pepijn de Geus added a comment - Thanks for your reply, Martin. Your commit fixes the problem, but has one disadvantage: the call to Connections.close(data.connection) still opens a new InputStream and immediately closes it. The problem is thereby even broader: every call to Connections.close(URLConnection) has this problem. Please refer to these implementations of URLConnection (in OpenJDK and GlassFish) to see a new stream created for every call: http://grepcode.com/file/repository.springsource.com/org.apache.geronimo/com.springsource.org.apache.geronimo.kernel/2.0.1/org/apache/geronimo/kernel/classloader/JarFileUrlConnection.java?av=h#JarFileUrlConnection http://grepcode.com/file/repo1.maven.org/maven2/org.glassfish.common/common-util/10.0-b28/com/sun/enterprise/loader/EJBClassLoader.java?av=h#EJBClassLoader.InternalJarURLConnection In my opinion all usages of the close(URLConnection) with connections other than HttpURLConnection are faulty and should be checked/changed. I'll attach a new patch (wicket-core-4293-2.patch) fixing the problem based on your new commit and extending the test you mentioned.
          Hide
          Martin Grigorov added a comment -

          The real problem is Connections#close() that calls getInputStream().
          I don't see a reason to do that.

          Show
          Martin Grigorov added a comment - The real problem is Connections#close() that calls getInputStream(). I don't see a reason to do that.
          Hide
          Pepijn de Geus added a comment -

          Exactly, but the streams that are used must of course be closed afterwards, which is what my patch accomplishes.

          Show
          Pepijn de Geus added a comment - Exactly, but the streams that are used must of course be closed afterwards, which is what my patch accomplishes.
          Hide
          Martin Grigorov added a comment -

          Your test improvement is not correct. Or at least doesn't follow the rule that calling getInputStream() should increase the streamCounter.
          I'll commit the improved version soon.

          Show
          Martin Grigorov added a comment - Your test improvement is not correct. Or at least doesn't follow the rule that calling getInputStream() should increase the streamCounter. I'll commit the improved version soon.
          Hide
          Pepijn de Geus added a comment -

          Thanks, I'm curiously waiting for your change
          I noted another potential problem in my patch; the last line of internalGetInputStream should read:

          return (data != null) ? data.inputStream : null;

          Show
          Pepijn de Geus added a comment - Thanks, I'm curiously waiting for your change I noted another potential problem in my patch; the last line of internalGetInputStream should read: return (data != null) ? data.inputStream : null;
          Hide
          Martin Grigorov added a comment -

          Hi Pepijn,

          Can you please take a look at r1215075 ?
          Thanks!

          Show
          Martin Grigorov added a comment - Hi Pepijn, Can you please take a look at r1215075 ? Thanks!
          Hide
          Pepijn de Geus added a comment -

          Hi Martin, that looks great. I see you have a different opinion on whether getInputStream should return a single stream instance or always a new one
          One remaining thing: you're still using Connections.closeQuietly(data.connection), which opens a new stream and immediately closes it. This is unnecessary and I'd suggest to fix that too by removing the call.
          Thanks!

          Show
          Pepijn de Geus added a comment - Hi Martin, that looks great. I see you have a different opinion on whether getInputStream should return a single stream instance or always a new one One remaining thing: you're still using Connections.closeQuietly(data.connection), which opens a new stream and immediately closes it. This is unnecessary and I'd suggest to fix that too by removing the call. Thanks!
          Hide
          Martin Grigorov added a comment -

          You didn't pay attention then
          Connections#close() no more does this.
          Thanks for your help!

          Show
          Martin Grigorov added a comment - You didn't pay attention then Connections#close() no more does this. Thanks for your help!
          Hide
          Pepijn de Geus added a comment -

          Ah sorry, didn't have the wicket-util project open. Shame on me.
          It works great now, thank you too

          Show
          Pepijn de Geus added a comment - Ah sorry, didn't have the wicket-util project open. Shame on me. It works great now, thank you too

            People

            • Assignee:
              Martin Grigorov
              Reporter:
              Pepijn de Geus
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development