Salut,
Bongjae Chang wrote:
> Hi Jeanfrancois,
>
> Jeanfrancois wrote:
>> I think what we want is
>>
>>
>> ThreadClassloader
>> / \
>> ServletAdapterA_CL ServletAdapterB_CL
>>
>> two ServletAdapter instance will have their own Classloader referencing
>> their web-inf/lib or classes directory, but they will share a common
>> based. Does it help?
>>
>
> I understood your word just now. You are right.
>
> Current ServletAdapter to which I applied the patch doesn't have your classloader hierarchy.
>
> The current classloader hierarchy is here.
> --------
> ServletAdapterA_CL's parent ServletAdapterB_CL's parent
> / \
> ServletAdapterA_CL ServletAdapterB_CL
> --------
>
> But, I also think that the parent classloader should be not their own Classloader's parent CL but common ThreadClassloader.
Agree.
>
> For your classloader hierarchy, maybe I think that we should make new ClassLoader and should override ContextLoader#loadClass() and findClass().
>
> Am I understanding it correctly?
Just to make sure, you propose to override those methods to isolate the
web application classes?
Thanks!
-- Jeanfrancois
>
> --
> Bongjae Chang
>
>
> ----- Original Message -----
> From: "Jeanfrancois Arcand" <Jeanfrancois.Arcand_at_Sun.COM>
> To: <dev_at_grizzly.dev.java.net>
> Sent: Saturday, August 29, 2009 2:41 AM
> Subject: Re: About ServletAdapter's classloader
>
>
>> Salut,
>>
>> Bongjae Chang wrote:
>>> 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?
>> I think what we want is
>>
>>
>> ThreadClassloader
>> / \
>> ServletAdapterA_CL ServletAdapterB_CL
>>
>> two ServletAdapter instance will have their own Classloader referencing
>> their web-inf/lib or classes directory, but they will share a common
>> based. Does it help?
>>
>>>
>>> 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?
>> Yes I would think so. Hubert, what do you think? We can merge later
>> unless you thunk it will breaks your work.
>>
>>
>>> 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.
>> +1
>>
>> Thanks
>>
>> -- Jeanfrancois
>>
>>> 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
>>>>
>>>>
>>>>
>>>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe_at_grizzly.dev.java.net
>> For additional commands, e-mail: dev-help_at_grizzly.dev.java.net
>>
>>
>>
>>