Hi Jeanfrancois,
Bongjae wrote:
>> 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.
Jeanfrancois wrote:
> 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.
Maybe I didn't understand your question correctly.
Could you please explain more?
Bongjae wrote:
>> But, I think that maybe this B)'s patch can have many regressions
>> if user's adapters(custom adapters) doesn't consider their classloader.
Jeanfrancois wrote:
> Can you elaborate about that one? I don't see where your two patches
> could conflict. IMO they are needed both.
Right. Maybe I was misunderstanding something yesterday. Please forget my words. :)
I know that grizzly deployer refactoring is in progress recently(It is great!). When I saw new grizzly deployer simply, it seemed that the WebAppAdapter would resolve this issue.
Though, do you think that it is better that my patch should be applyed to current version?
If yes, I will open two issue trackers(1. About ServletAdapter's own classloader, 2. About HttpWorkerThreadFactory's setContextClassLoader()) and commit them.
Please let me know.
Thanks!
--
Bongjae Chang
----- Original Message -----
From: "Jeanfrancois Arcand" <Jeanfrancois.Arcand_at_Sun.COM>
To: <dev_at_grizzly.dev.java.net>
Sent: Friday, August 28, 2009 12:25 AM
Subject: Re: About ServletAdapter's classloader
> 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
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>
>
>
>