Craig,
Well-stated, and I hope the audience takes the time to read it--I am
seeing tons os thread-unsafe methods, fixed about 15 already.
Lloyd
On Dec 7, 2006, at 5:38 PM, Craig L Russell wrote:
> This is a very good use case for understanding static initialization.
>
> My understanding is that if you have a static initializer such as:
>
> private static WebServiceEngineImpl INSTANCE = new
> WebServiceEngineImpl();
>
> then you can use a non-synchronized accessor,
>
> public static /*synchronized*/ WebServiceEngineImpl getInstance
> () {
> return INSTANCE;
> }
>
> because initialization of a static is a special case of thread-safe
> access.
>
> But this will completely initialize the WebServiceEngineImpl
> instance before completing the initialization of the caller,
> including the static initialization of the WebServiceEngineImpl
> class itself.
>
> If you want to lazily initialize a component you must use the
> synchronization primitive either explicitly with the synchronized
> { ... } or the synchronized keyword on the method, as
>
> public static synchronized WebServiceEngineFactory getInstance() {
> if (_factory==null) {
> _factory = new WebServiceEngineFactory();
> }
> return _factory;
> }
>
> This will synchronize every thread's access to the
> WebServiceEngineFactory as a tradeoff for the ability to lazily
> initialize it. The tradeoff means that you should consider whether
> a component will always be needed (use static initializer) or might
> not be needed (use lazy initialization).
>
> Craig
>
> P.S. I'm glad to see that no one suggested using the broken double
> checked lock as a solution!
>
> On Dec 7, 2006, at 2:28 PM, Lloyd L Chambers wrote:
>
>> Yes, thanks, my oversight.
>>
>> On Dec 7, 2006, at 1:49 PM, Dhiru Pandey wrote:
>>
>>> 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.LogAuthenticationListene
>>>>>> r;
>>>>>> +
>>>>>> /**
>>>>>> * 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
>>>>
>>
>
> Craig Russell
> Architect, Sun Java Enterprise System http://java.sun.com/products/jdo
> 408 276-5638 mailto:Craig.Russell_at_sun.com
> P.S. A good JDO? O, Gasp!
>