dev@glassfish.java.net

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

From: Dhiru Pandey <Dhiru.Pandey_at_Sun.COM>
Date: Thu, 07 Dec 2006 13:49:24 -0800

Hello Lloyd,

Looks fine but really no need for synchronized in :

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

Also, you may want to do the same thing for:

    public static synchronized WebServiceEngineFactory getInstance() {
        if (_factory==null) {
            _factory = new WebServiceEngineFactory();
        }
        return _factory;
    }


Lloyd L Chambers wrote:
> 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
>>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe_at_glassfish.dev.java.net
> For additional commands, e-mail: dev-help_at_glassfish.dev.java.net
>