glassfish
  1. glassfish
  2. GLASSFISH-18806

Thread used to process AdminAdapter request has context class loader set

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 4.0_b41
    • Fix Version/s: 4.0_b71
    • Component/s: grizzly-kernel
    • Labels:
      None

      Description

      The AdminAdapter is a Grizzly Adapter. When the onMissingResource method is called, the context class loader is already set in the Thread. It isn't clear where this is being set. Is it from some previous request that used that thread? Is Grizzly setting this?

      The class loader is a Felix class loader.

      It seems that a thread used to process a Grizzly request should not have the context class loader already set.

        Activity

        Tom Mueller created issue -
        Hide
        oleksiys added a comment -

        it happens on admin port, right?
        does admin console use Grizzly "servlet" container?

        Show
        oleksiys added a comment - it happens on admin port, right? does admin console use Grizzly "servlet" container?
        Hide
        Tom Mueller added a comment -

        Yes, this is on the admin port.
        AdminAdapter extends org.glassfish.grizzly.http.server.StaticHttpHandler

        Sorry for the reference to "Grizzly Adapter" above, that should have been Grizzly StaticHttpHandler.

        Show
        Tom Mueller added a comment - Yes, this is on the admin port. AdminAdapter extends org.glassfish.grizzly.http.server.StaticHttpHandler Sorry for the reference to "Grizzly Adapter" above, that should have been Grizzly StaticHttpHandler.
        Hide
        Ryan Lubke added a comment -

        I've verified that the only location that Grizzly ever calls setContextClassLoader() is within our Servlet container code, which doesn't appear to be used here.

        Given this, I've set a break point in Thread.setContextClassLoader() and have found the following:

        • initial request received; context classloader set to felix classloader by HK2Dispatcher

        at com.sun.enterprise.v3.server.HK2Dispatcher.dispatch(HK2Dispatcher.java:111)
        at com.sun.enterprise.v3.services.impl.ContainerMapper.service(ContainerMapper.java:236)
        at org.glassfish.grizzly.http.server.HttpHandler.doHandle(HttpHandler.java:164)
        at org.glassfish.grizzly.http.server.HttpServerFilter.handleRead(HttpServerFilter.java:169)

        • the AdminAdapter is invoked by the dispatch call:

        at com.sun.enterprise.v3.admin.AdminAdapter.onMissingResource(AdminAdapter.java:232)
        at org.glassfish.grizzly.http.server.StaticHttpHandler.service(StaticHttpHandler.java:298)
        at com.sun.enterprise.v3.server.HK2Dispatcher.dispatch(HK2Dispatcher.java:113)
        at com.sun.enterprise.v3.services.impl.ContainerMapper.service(ContainerMapper.java:236)

        Show
        Ryan Lubke added a comment - I've verified that the only location that Grizzly ever calls setContextClassLoader() is within our Servlet container code, which doesn't appear to be used here. Given this, I've set a break point in Thread.setContextClassLoader() and have found the following: initial request received; context classloader set to felix classloader by HK2Dispatcher at com.sun.enterprise.v3.server.HK2Dispatcher.dispatch(HK2Dispatcher.java:111) at com.sun.enterprise.v3.services.impl.ContainerMapper.service(ContainerMapper.java:236) at org.glassfish.grizzly.http.server.HttpHandler.doHandle(HttpHandler.java:164) at org.glassfish.grizzly.http.server.HttpServerFilter.handleRead(HttpServerFilter.java:169) the AdminAdapter is invoked by the dispatch call: at com.sun.enterprise.v3.admin.AdminAdapter.onMissingResource(AdminAdapter.java:232) at org.glassfish.grizzly.http.server.StaticHttpHandler.service(StaticHttpHandler.java:298) at com.sun.enterprise.v3.server.HK2Dispatcher.dispatch(HK2Dispatcher.java:113) at com.sun.enterprise.v3.services.impl.ContainerMapper.service(ContainerMapper.java:236)
        Hide
        Tom Mueller added a comment -

        Ryan, thanks for finding this.
        I see that HK2Dispatcher just set the thread context class loader to be the class loader for the adapter class. Any idea why it does this?

        Show
        Tom Mueller added a comment - Ryan, thanks for finding this. I see that HK2Dispatcher just set the thread context class loader to be the class loader for the adapter class. Any idea why it does this?
        Hide
        Ryan Lubke added a comment -

        I can't really say. This is Jerome's code - and based on svn annotation output, this class has remained mostly unchanged since '07.

        Show
        Ryan Lubke added a comment - I can't really say. This is Jerome's code - and based on svn annotation output, this class has remained mostly unchanged since '07.
        Ryan Lubke made changes -
        Field Original Value New Value
        Assignee oleksiys [ oleksiys ] Tom Mueller [ tmueller ]
        Ryan Lubke made changes -
        Component/s other [ 10611 ]
        Component/s grizzly-kernel [ 10632 ]
        Hide
        Tom Mueller added a comment -

        Reassigning to the hk2 component.

        The HK2Dispatcher class (in the nucleus/core/kernel) module is setting the context class loader for requests that are handled by an HttpAdapter to the adapter class loader even when the context class loader that is passed in for that class is null. It isn't clear why the HK2Dispatcher class is doing this. It isn't even clear why we have the HK2Dispatcher class, since most of it is commented out now.

        Please evaluate whether HK2Dispatcher is still needed and if it isn't, then delete it.

        Show
        Tom Mueller added a comment - Reassigning to the hk2 component. The HK2Dispatcher class (in the nucleus/core/kernel) module is setting the context class loader for requests that are handled by an HttpAdapter to the adapter class loader even when the context class loader that is passed in for that class is null. It isn't clear why the HK2Dispatcher class is doing this. It isn't even clear why we have the HK2Dispatcher class, since most of it is commented out now. Please evaluate whether HK2Dispatcher is still needed and if it isn't, then delete it.
        Tom Mueller made changes -
        Assignee Tom Mueller [ tmueller ] michael.y.chen [ michael.y.chen ]
        Component/s hk2 [ 14436 ]
        Component/s other [ 10611 ]
        Tom Mueller made changes -
        Assignee michael.y.chen [ michael.y.chen ] jwells [ jwells ]
        jwells made changes -
        Component/s grizzly-kernel [ 10632 ]
        Component/s hk2 [ 14436 ]
        Hide
        jwells added a comment -

        I have removed HK2Dispatcher. However, I replicated the code that was setting the CCL in ContainerMapper since I did not want to break anything. However, that should make it clear that it has nothing to do with HK2 anymore.

        If you look at ContainerMapper you will see the method I added. It does make the logic more clear and obviously can now be handled directly in the ContainerMapper without worrying about how it would affect anything else.

        Show
        jwells added a comment - I have removed HK2Dispatcher. However, I replicated the code that was setting the CCL in ContainerMapper since I did not want to break anything. However, that should make it clear that it has nothing to do with HK2 anymore. If you look at ContainerMapper you will see the method I added. It does make the logic more clear and obviously can now be handled directly in the ContainerMapper without worrying about how it would affect anything else.
        jwells made changes -
        Assignee jwells [ jwells ] Ryan Lubke [ rlubke ]
        Hide
        jwells added a comment -

        I would also like to add that I have no idea why this code used to set the CCL to the adapter if no ClassLoader was passed to it. To me that doesn't seem correct in this instance...

        Show
        jwells added a comment - I would also like to add that I have no idea why this code used to set the CCL to the adapter if no ClassLoader was passed to it. To me that doesn't seem correct in this instance...
        Hide
        Ryan Lubke added a comment -

        After a brief discussion with Tom, we both agree we'll probably never really know the purpose of this code and to go ahead and try to remove it.

        Show
        Ryan Lubke added a comment - After a brief discussion with Tom, we both agree we'll probably never really know the purpose of this code and to go ahead and try to remove it.
        Hide
        Ryan Lubke added a comment -

        Sending nucleus/core/kernel/src/main/java/com/sun/enterprise/v3/services/impl/ContainerMapper.java
        Transmitting file data .
        Committed revision 58191.

        Show
        Ryan Lubke added a comment - Sending nucleus/core/kernel/src/main/java/com/sun/enterprise/v3/services/impl/ContainerMapper.java Transmitting file data . Committed revision 58191.
        Ryan Lubke made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Fix Version/s 4.0_b71 [ 16100 ]
        Resolution Fixed [ 1 ]

          People

          • Assignee:
            Ryan Lubke
            Reporter:
            Tom Mueller
          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: