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. Wicket153-bug.zip
        22 kB
        Pepijn de Geus
      2. patch.txt
        2 kB
        Pepijn de Geus
      3. wicket-core-4293-2.patch
        6 kB
        Pepijn de Geus

        Issue Links

          Activity

          Pepijn de Geus created issue -
          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.
          Pepijn de Geus made changes -
          Field Original Value New Value
          Attachment Wicket153-bug.zip [ 12507356 ]
          Attachment patch.txt [ 12507357 ]
          Martin Grigorov committed 1214729 (1 file)
          Reviews: none

          WICKET-4293
          UrlResourceStream closes incorrect InputStream causing stacktraces on undeploy on GlassFish

          Cache the input stream to be able to close it when the connection creates a new stream for each call of #getInputStream().

          Martin Grigorov committed 1214733 (1 file)
          Reviews: none

          WICKET-4293
          UrlResourceStream closes incorrect InputStream causing stacktraces on undeploy on GlassFish

          Cache the input stream to be able to close it when the connection creates a new stream for each call of #getInputStream().

          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!
          Martin Grigorov made changes -
          Status Open [ 1 ] Resolved [ 5 ]
          Assignee Martin Grigorov [ mgrigorov ]
          Fix Version/s 1.5.4 [ 12319051 ]
          Fix Version/s 6.0.0 [ 12315431 ]
          Resolution Fixed [ 1 ]
          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.
          Pepijn de Geus made changes -
          Attachment wicket-core-4293-2.patch [ 12507535 ]
          Martin Grigorov made changes -
          Resolution Fixed [ 1 ]
          Status Resolved [ 5 ] Reopened [ 4 ]
          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;
          Martin Grigorov committed 1215075 (3 files)
          Reviews: none

          WICKET-4293
          UrlResourceStream closes incorrect InputStream causing stacktraces on undeploy on GlassFish

          Keep a list of all acquired InputStreams and close them when UrlResourceStream#close() is invoked.

          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
          Martin Grigorov committed 1215138 (1 file)
          Reviews: none

          WICKET-4293
          UrlResourceStream closes incorrect InputStream causing stacktraces on undeploy on GlassFish

          Martin Grigorov made changes -
          Status Reopened [ 4 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Martin Grigorov made changes -
          Link This issue is duplicated by WICKET-3957 [ WICKET-3957 ]

            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