Hi,
GWSD refactoring was not touching ServletAdapter so it's fine to patch it
and merging should be easy.
Adding SA.setClassLoader() will help me with refactoring :), I wanted to
maintain CLs in WebAppAdapters, now I think
One thing I'm not sure is what would be semantics of setting ClassLoader on
started ServletAdapter, if it should be allowed, and if we should worry
about it at all?
I really like your patch and think that it could be even
more simplified by generating classLoader if none was provided
at beginning of SA.start() so we always have classLoader and can remove all
conditional processing.
Kudos Bongjae,
Hubert.
On Mon, Aug 31, 2009 at 6:55 AM, Bongjae Chang <carryel_at_korea.com> wrote:
> Thank you for your reply, Jeanfrancois.
>
> And Hubert, please let me know your thoughts. :)
>
> Thanks!
> --
> 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
> >
> >
> >
> >
>