Salut,
Bongjae Chang wrote:
> Hi,
>
> When I tested GrizzlyWebServer and GrizzlyWebServerDeployer variously, I
> found some interesting points.
>
> When I deployed an web application after GrizzlyWebServer.start(),
> sometimes the web application didn't work well.
>
> The problem was that my web application didn't access own resources.
> (Many web applications used to load resources from current thread's
> ContextLoader)
>
> But, when I deployed an web application after GrizzlyWebServer.start()
> was called, it worked fine.
>
> When I debugged it, it seemed that this problem had been caused by
> current thread's ContextLoader.
>
> When GrizzlyWebServer(gws) was started, the gws initialized the thread
> pool. (See the SelectorThread#initThreadPool() ).
>
> Default thread pool is StatsThreadPool and default ThreadFactory is
> HttpWorkerThreadFactory.
>
> When HttpWorkerThreadFactory creates new thread, HttpWorkerThreadFactory
> doesn't set thread's ContextClassLoader.
That's an issue.
>
> Then, threads of the thread pool will use the class loader where
> gws.start() is executed as ContextClassLoader because the
> ContextClassLoader is propagated from the parent thread when new thread
> is created.
>
> And the threads' ContextClassLoader will be served when clients request
> the service.
>
> So I understood my problems. Actually when I met errors in my test, the
> serviced ContextClassLoader(Thread.currentThread().getContextClassLoader())
> is not the application class loader which I created.
>
> If users would like to deploy applications like using
> gws.addGrizzlyAdapter() after gws has started, it seems that current
> grizzly can't support them correctly because the adapter doesn't have
> own classloader.
>
> A) So, I think that it is better that the adapter has own classloader.
+1
>
> Moreover, when I used GrizzlyWebServerDeployer, I could see the
> duplicated classloader hierarchy.
>
> For examples,
>
> Assuming that the "example" web application has the following libraries,
> ---
> /example/WEB-INF/lib/a.jar
> /example/WEB-INF/lib/b.jar
> /example/WEB-INF/classes/c.class
> /example/WEB-INF/classes/d.class
> ---
>
> GrizzlyWebServerDeploy will make new URLClassLoader*(1)* which has
> a.jar, b.jar, c.class and d.class before GrizzlyWebServerDeployer
> deploys the app and set the URLClassLoader to be current thread's
> ContextLoader.
>
> Then, an ServletAdaper will be added to GrizzlyWebServer and the adapter
> will be started. See the following code.
>
> In ServletAdapter.java
> ---
> public void start() {
> try{
> if (initialize) {
> initWebDir();
> configureClassLoader(webDir.getCanonicalPath());
> }
> } catch(...) {
> ...
> }
> }
>
> protected void configureClassLoader(String applicationPath) throws
> IOException{
>
> Thread.currentThread().setContextClassLoader(ClassLoaderUtil.createURLClassLoader(applicationPath));
> --- *(2)*
> }
> ---
>
> GrizzlyWebServerDeploy already set the application classloader but
> ServletAdapter always creates new URLClassLoader with
> ClassLoaderUtil.createURLClassLoader().
>
> Then, the classloader hierarchy is here.
> ---
> GrizzlyWebServerDeployer's app classloader *(1)* --- parent classloader
> |
> ServletAdaper's app classloader *(2)*
> ---
>
> (1) and (2) classloaders have same class path.
>
>
> B) So, I think that if the ServletAdapter already has the app
> classloader explicitly, ServletAdapter need not to create new app
> classloader.
Agree.
>
>
> Here is the my example patch about (A) and (B).
>
> Index: com/sun/grizzly/http/servlet/ServletAdapter.java
> ===================================================================
> --- com/sun/grizzly/http/servlet/ServletAdapter.java (revision 3546)
> +++ com/sun/grizzly/http/servlet/ServletAdapter.java (working copy)
> @@ -172,6 +172,8 @@
> * Initialize the {_at_link <mailto:{_at_link> ServletContext}
> */
> protected boolean initialize = true;
> +
> + protected ClassLoader classLoader;
>
>
> public ServletAdapter() {
> @@ -274,11 +276,26 @@
> initWebDir();
> configureClassLoader(webDir.getCanonicalPath());
> }
> - configureServletEnv();
> - fullUrlPath = contextPath + servletPath;
> - setResourcesContextPath(fullUrlPath);
> - if (loadOnStartup){
> - loadServlet();
> + if( classLoader != null ) {
> + ClassLoader prevClassLoader =
> Thread.currentThread().getContextClassLoader();
> + Thread.currentThread().setContextClassLoader(
> classLoader );
> + try {
> + configureServletEnv();
> + fullUrlPath = contextPath + servletPath;
> + setResourcesContextPath( fullUrlPath );
> + if( loadOnStartup ) {
> + loadServlet();
> + }
> + } finally {
> + Thread.currentThread().setContextClassLoader(
> prevClassLoader );
> + }
> + } else {
> + configureServletEnv();
> + fullUrlPath = contextPath + servletPath;
> + setResourcesContextPath( fullUrlPath );
> + if( loadOnStartup ) {
> + loadServlet();
> + }
> }
> } catch (Throwable t){
> logger.log(Level.SEVERE,"start",t);
> @@ -294,8 +311,8 @@
> * @throws java.io.IOException I/O error.
> */
> protected void configureClassLoader(String applicationPath) throws
> IOException{
> - Thread.currentThread().setContextClassLoader(
> - ClassLoaderUtil.createURLClassLoader(applicationPath));
> + if( classLoader == null )
> + classLoader =
> ClassLoaderUtil.createURLClassLoader(applicationPath);
> }
>
>
> @@ -304,6 +321,20 @@
> */
> @Override
> public void service(GrizzlyRequest request, GrizzlyResponse response) {
> + if( classLoader != null ) {
> + ClassLoader prevClassLoader =
> Thread.currentThread().getContextClassLoader();
> + Thread.currentThread().setContextClassLoader( classLoader );
> + try {
> + doService( request, response );
> + } finally {
> + Thread.currentThread().setContextClassLoader(
> prevClassLoader );
> + }
> + } else {
> + doService( request, response );
> + }
> + }
> +
> + public void doService(GrizzlyRequest request, GrizzlyResponse
> response) {
> try {
> Request req = request.getRequest();
> Response res = response.getResponse();
> @@ -679,9 +710,21 @@
> */
> @Override
> public void destroy(){
> - super.destroy();
> - servletCtx.destroyListeners();
> - filterChain.destroy();
> + if( classLoader != null ) {
> + ClassLoader prevClassLoader =
> Thread.currentThread().getContextClassLoader();
> + Thread.currentThread().setContextClassLoader( classLoader );
> + try {
> + super.destroy();
> + servletCtx.destroyListeners();
> + filterChain.destroy();
> + } finally {
> + Thread.currentThread().setContextClassLoader(
> prevClassLoader );
> + }
> + } else {
> + super.destroy();
> + servletCtx.destroyListeners();
> + filterChain.destroy();
> + }
> }
>
>
> @@ -697,6 +740,8 @@
> ServletAdapter sa = new ServletAdapter(".",servletCtx,
> contextParameters,
> new HashMap<String,String>(), listeners,
> false);
> + if( classLoader != null )
> + sa.setClassLoader( classLoader );
> sa.setServletInstance(servlet);
> sa.setServletPath(servletPath);
> return sa;
> @@ -713,4 +758,12 @@
> protected HashMap<String, String> getContextParameters() {
> return contextParameters;
> }
> +
> + public ClassLoader getClassLoader() {
> + return classLoader;
> + }
> +
> + public void setClassLoader( ClassLoader classLoader ) {
> + this.classLoader = classLoader;
> + }
> }
>
>
> Then the adapter will use the user's classloader instead of thread
> pool's classloader when a user creates an adapter like this.
So far I really like your patch!
>
> ---
> ServletAdapter sa = new ServletAdapter();
> *sa.setClassLoader( userClassLoader );*
> sa.setContextPath( contextPath );
> sa.setServletPath( servletPath );
> sa.setRootFolder( rootFolder );
> ...
> ---
>
> If an user doesn't set the classloader of ServletAdapter, I think that
> the behavior will be similar to old version's behavior.
Agree.
>
>
>
> Another issue is about HttpWorkerThreadFactory.
>
> Current HttpWorkerThreadFactory doesn't set the ContextClassLoader. Then
> the current ContextClassLoader of executing the thread pool will be used
> when the thread pool creates new threads as I said above.
>
> C) But I think that this way is not safe because we can't estimate
> when new thread will be created and what is current thread's ContextLoader.
But Should all the newly created Thread share the *same*
ContextClassLoader? The ServletAdapter must have its own, but the parent
should have the same IMO.
>
> If current thread's ContextLoader is user's application classloader,
> memory leak can be occurred because the thread pool can preserve the
> classloader after the application already undeployed. (But, I am not sure)
Yes usually I saw that issue with Tomcat and GlassFish...many many times.
>
> So, I think that the following patch is better about C).
>
> Index: com/sun/grizzly/http/StatsThreadPool.java
> ===================================================================
> --- com/sun/grizzly/http/StatsThreadPool.java (revision 3546)
> +++ com/sun/grizzly/http/StatsThreadPool.java (working copy)
> @@ -41,6 +41,8 @@
> import java.util.concurrent.LinkedBlockingQueue;
> import java.util.concurrent.ThreadFactory;
> import java.util.concurrent.TimeUnit;
> +import java.security.AccessController;
> +import java.security.PrivilegedAction;
>
> /**
> * Internal FIFO used by the Worker Threads to pass information
> @@ -116,14 +118,21 @@
> * Create new {_at_link <mailto:{_at_link> HttpWorkerThread}.
> */
> protected class HttpWorkerThreadFactory implements ThreadFactory {
> - public Thread newThread(Runnable r) {
> - Thread thread = new HttpWorkerThread(StatsThreadPool.this,
> - name + port + "-WorkerThread(" +
> - workerThreadCounter.getAndIncrement() + ")", r,
> - initialByteBufferSize);
> - thread.setUncaughtExceptionHandler(StatsThreadPool.this);
> - thread.setPriority(priority);
> - return thread;
> + public Thread newThread(final Runnable r) {
> + return AccessController.doPrivileged(
> + new PrivilegedAction<Thread>() {
> + public Thread run() {
> + Thread thread = new HttpWorkerThread(
> StatsThreadPool.this,
> + name
> + port + "-WorkerThread(" +
> +
> workerThreadCounter.getAndIncrement() + ")", r,
> +
> initialByteBufferSize );
> + thread.setUncaughtExceptionHandler(
> StatsThreadPool.this );
> + thread.setPriority( priority );
> + thread.setContextClassLoader(
> HttpWorkerThreadFactory.class.getClassLoader() );
> + return thread;
> + }
> + }
> + );
> }
> }
>
> But, I think that maybe this B)'s patch can have many regressions
> if user's adapters(custom adapters) doesn't consider their classloader.
Can you elaborate about that one? I don't see where your two patches
could conflict. IMO they are needed both.
>
> And the A)'s patch only considers the ServletAdapter, but GrizzlyAdapter
> and StaticResourcesAdapter can also have same issues.( I am not sure).
Both above class don't need to load classes...so I would think we are
safe IMO.
>
> Thank you for reading my long mail contents!
Quite good analysis as usual! Send more analysis like that one!
A+
--Jeanfrancois
>
> --
> Bongjae Chang