dev@glassfish.java.net

Re: CODE REVIEW: WebServiceEngine thread-unsafe initialization bugs #1694, #1695

From: Lloyd L Chambers <Lloyd.Chambers_at_Sun.COM>
Date: Thu, 07 Dec 2006 13:09:16 -0800

Dhiru,

Your suggestion is a good one. Assuming the class is never loaded
before it's needed, using a static initializer is not wasteful.

The original code was quite tricky (two intertwined classes) and your
suggestion wasn't feasible. I've simplified it down to this now:

public class WebServiceEngineImpl implements WebServiceEngine {

     /** Creates a new instance of TracingSystemHandlerFactory */
     protected WebServiceEngineImpl() {
         addAuthListener( new LogAuthenticationListener() );
     }

     private static final WebServiceEngineImpl INSTANCE = new
WebServiceEngineImpl();

     public static synchronized WebServiceEngineImpl getInstance() {
         return INSTANCE;
     }
.......
}


public class WebServiceEngineFactory {

     private static WebServiceEngineFactory _factory = null;

     private final WebServiceEngine engine;

     /** Creates a new instance of WebServiceEngineFactory */
     protected WebServiceEngineFactory() {
         // build-order problem--have to use reflection
         try {
             final ClassLoader classLoader = Thread.currentThread
().getContextClassLoader();
             final Class engineClass =
                 classLoader.loadClass
( "com.sun.enterprise.webservice.monitoring.WebServiceEngineImpl" );
             final java.lang.reflect.Method m =
engineClass.getDeclaredMethod( "getInstance", (Class[])null);
             engine = (WebServiceEngine)m.invoke( null, (Object[])
null );
         }
         catch( final Exception e ) {
             throw new Error( e );
         }
     }

     /**
      * @return the singleton Factory implementation
      */
     public static synchronized WebServiceEngineFactory getInstance() {
         if (_factory==null) {
             _factory = new WebServiceEngineFactory();
         }
         return _factory;
     }

     /**
      * @return this appserver WebServiceEngine instance
      */
     public WebServiceEngine getEngine() {
         return engine;
     }
}


Lloyd

On Dec 7, 2006, at 11:40 AM, Dhiru Pandey wrote:

> Why not do this:
>
> private static WebServiceEngineImpl INSTANCE = new
> WebServiceEngineImpl();
>
> public static WebServiceEngineImpl getInstance() {
> return INSTANCE;
> }
>
> instead of:
>
> + private static WebServiceEngineImpl INSTANCE = null;
> +
> + public static synchronized WebServiceEngineImpl getInstance() {
> + if ( INSTANCE == null ) {
> + INSTANCE = new WebServiceEngineImpl();
> }
> - return (WebServiceEngineImpl) factory.getEngine();
> + return INSTANCE;
> }
>
> You would save on the synchronization cost if getInstance() call is
> called by multiple threads
>
> -Dhiru
>
> Lloyd L Chambers wrote:
>> Jerome,
>>
>> Thanks...
>>
>> But it seems I was hasty--it is more complicated due to an
>> interaction between the factory and the impl (see my other
>> email). And there is a build-order problem too.
>>
>> Here are new diffs which fix the problem I'm seeing with PE. Is
>> there a different approach going on in EE? (it doesn't appear so
>> as there is no -D business going on).
>>
>> MB2:/gf/build/glassfish lloyd$ cvs diff -u appserv-core/src/java/
>> com/sun/enterprise/webservice/monitoring/WebServiceEngineImpl.java
>> appserv-commons/src/java/com/sun/enterprise/webservice/monitoring/
>> WebServiceEngineFactory.java
>> Index: appserv-core/src/java/com/sun/enterprise/webservice/
>> monitoring/WebServiceEngineImpl.java
>> ===================================================================
>> RCS file: /cvs/glassfish/appserv-core/src/java/com/sun/enterprise/
>> webservice/monitoring/WebServiceEngineImpl.java,v
>> retrieving revision 1.14
>> diff -u -r1.14 WebServiceEngineImpl.java
>> --- appserv-core/src/java/com/sun/enterprise/webservice/monitoring/
>> WebServiceEngineImpl.java 18 Nov 2006 02:48:20 -0000 1.14
>> +++ appserv-core/src/java/com/sun/enterprise/webservice/monitoring/
>> WebServiceEngineImpl.java 7 Dec 2006 18:12:17 -0000
>> @@ -58,6 +58,8 @@
>> import com.sun.logging.LogDomains;
>> import com.sun.enterprise.deployment.WebServiceEndpoint;
>> +import
>> com.sun.enterprise.webservice.monitoring.LogAuthenticationListener;
>> +
>> /**
>> * This class acts as a factory to create TracingSystemHandler
>> * instances. It also provides an API to register listeners
>> @@ -74,27 +76,22 @@
>> new ArrayList<AuthenticationListener>();
>> protected GlobalMessageListener globalMessageListener = null;
>>
>> -
>> - static WebServiceEngineFactory factory =
>> WebServiceEngineFactory.getInstance();
>> static ThreadLocal servletThreadLocal = new ThreadLocal();
>> public static Logger sLogger = LogDomains.getLogger
>> (LogDomains.CORE_LOGGER);
>>
>>
>> /** Creates a new instance of TracingSystemHandlerFactory */
>> protected WebServiceEngineImpl() {
>> + addAuthListener( new LogAuthenticationListener() );
>> }
>>
>> - public static WebServiceEngineImpl getInstance() {
>> -
>> - if (factory.getEngine()==null) {
>> - synchronized(factory) {
>> - if (factory.getEngine()==null) {
>> - factory.setEngine(new WebServiceEngineImpl());
>> - factory.getEngine().addAuthListener(new
>> LogAuthenticationListener());
>> - }
>> - }
>> + private static WebServiceEngineImpl INSTANCE = null;
>> +
>> + public static synchronized WebServiceEngineImpl getInstance() {
>> + if ( INSTANCE == null ) {
>> + INSTANCE = new WebServiceEngineImpl();
>> }
>> - return (WebServiceEngineImpl) factory.getEngine();
>> + return INSTANCE;
>> }
>>
>> public EndpointImpl createHandler(WebServiceEndpoint
>> endpointDesc) {
>> Index: appserv-commons/src/java/com/sun/enterprise/webservice/
>> monitoring/WebServiceEngineFactory.java
>> ===================================================================
>> RCS file: /cvs/glassfish/appserv-commons/src/java/com/sun/
>> enterprise/webservice/monitoring/WebServiceEngineFactory.java,v
>> retrieving revision 1.3
>> diff -u -r1.3 WebServiceEngineFactory.java
>> --- appserv-commons/src/java/com/sun/enterprise/webservice/
>> monitoring/WebServiceEngineFactory.java 25 Dec 2005 04:12:42
>> -0000 1.3
>> +++ appserv-commons/src/java/com/sun/enterprise/webservice/
>> monitoring/WebServiceEngineFactory.java 7 Dec 2006 18:12:17
>> -0000
>> @@ -20,10 +20,10 @@
>> *
>> * Copyright 2006 Sun Microsystems, Inc. All rights reserved.
>> */
>> -
>> -
>> package com.sun.enterprise.webservice.monitoring;
>> +import java.lang.reflect.Method;
>> +
>> /**
>> * Factory for the WebServiceEngine
>> *
>> @@ -31,26 +31,31 @@
>> */
>> public class WebServiceEngineFactory {
>>
>> - private static WebServiceEngineFactory factory = null;
>> + private static WebServiceEngineFactory _factory = null;
>>
>> private WebServiceEngine engine = null;
>>
>> /** Creates a new instance of WebServiceEngineFactory */
>> protected WebServiceEngineFactory() {
>> + // build-order problem
>> + try {
>> + final Class engineClass = Class.forName
>> ( "com.sun.enterprise.webservice.monitoring.WebServiceEngineImpl" );
>> + final java.lang.reflect.Method m =
>> engineClass.getDeclaredMethod( "getInstance", (Class[])null);
>> + engine = (WebServiceEngine)m.invoke( null, (Object[])
>> null );
>> + }
>> + catch( final Exception e ) {
>> + throw new Error( e );
>> + }
>> }
>>
>> /**
>> * @return the singleton Factory implementation
>> */
>> - public static WebServiceEngineFactory getInstance() {
>> - if (factory==null) {
>> - synchronized(WebServiceEngineFactory.class) {
>> - if (factory==null) {
>> - factory = new WebServiceEngineFactory();
>> - }
>> - }
>> - }
>> - return factory;
>> + public static synchronized WebServiceEngineFactory getInstance
>> () {
>> + if (_factory==null) {
>> + _factory = new WebServiceEngineFactory();
>> + }
>> + return _factory;
>> }
>>
>> /**
>>
>>
>>
>>
>>
>> On Dec 7, 2006, at 9:25 AM, Jerome Dochez wrote:
>>
>>> looks good
>>> On Dec 7, 2006, at 8:56 AM, Lloyd L Chambers wrote:
>>>
>>>> See diffs in bug:
>>>>
>>>> https://glassfish.dev.java.net/issues/show_bug.cgi?id=1685
>>>>
>>>> Lloyd Chambers
>>>>
>>>> -------------------------------------------------------------------
>>>> --
>>>> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
>>>> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>>>>
>>>
>>> --------------------------------------------------------------------
>>> -
>>> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
>>> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
>> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>>