Yes, I agree....I heard Brian Goetz talk, and immediately purchased
(with personal funds!) "Java Concurrency in Practice".
A must-read for anyone working on AppServer....MANAGERS have you
purchased a copy for each of your engineers?! Everyone should have a
copy on hand at all times; some of this stuff is tricky especially
when optimizing or threading existing code.
I hadn't really understood the visibility aspect, but it is highly
relevant to our code.
I think all code reviews need to take concurrency into account.
Lloyd
On Dec 8, 2006, at 7:14 PM, Craig L Russell wrote:
> Hi Lloyd,
>
> Thanks, I recently had the great pleasure to hear one of the
> experts on the Java Memory Model, Bill Pugh, talk about the updates
> to the Model, and I learned a lot. In particular, I had not
> realized that static initialization of a static instance was
> guaranteed to be thread-safe. That one point alone was worth the
> price of attendance.
>
> Craig
>
> On Dec 8, 2006, at 1:01 PM, Lloyd L Chambers wrote:
>
>> Craig,
>>
>> Well-stated, and I hope the audience takes the time to read it--I
>> am seeing tons of 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.LogAuthenticationListe
>>>>>>>> ner;
>>>>>>>> +
>>>>>>>> /**
>>>>>>>> * 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.WebServiceEngineImp
>>>>>>>> l" );
>>>>>>>> + 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!
>>>
>>
>
> 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!
>