Needless synchronization?

[Brad Plies]

While reviewing how OFBIZ handled email verifications I observed the synchronized code below.  I do not understand why it is synchronized and finding the answers as to why will greatly help me better understand the inner workings of OFBIZ.


in ContactMechServices.createEmailAddressVerification
Line 1057 - 1082

        synchronized(ContactMechServices.class) {
            while(true){
                Long random = secureRandom.nextLong();
                verifyHash = HashCrypt.getDigestHash(Long.toString(random), "MD5");
                List<GenericValue> emailAddVerifications = null;
                try {
                    emailAddVerifications = delegator.findByAnd("EmailAddressVerification", UtilMisc.toMap("verifyHash", verifyHash));
                } catch (GenericEntityException e) {
                    Debug.logError(e.getMessage(), module);
                    return ServiceUtil.returnError(e.getMessage());
                }
                if(UtilValidate.isEmpty(emailAddVerifications)) {
                    GenericValue emailAddressVerification = delegator.makeValue("EmailAddressVerification");
                    emailAddressVerification.set("emailAddress", emailAddress);
                    emailAddressVerification.set("verifyHash", verifyHash);
                    emailAddressVerification.set("expireDate", expireDate);
                    try {
                        delegator.create(emailAddressVerification);
                    } catch (GenericEntityException e) {
                        Debug.logError(e.getMessage(),module);
                        return ServiceUtil.returnError(e.getMessage());
                    }
                    break;
                }
            }
        }


Why is there synchronization here?  
I can only find synch like this in 2 other places in OFBIZ applications.  There are about 13 instances of this type of synch in the OFBIZ framework (in some Utils & Crypt).

What is the shared state being protected?  Or what code must be guaraunteed to be running in no more than one thread at a time?
I can only guess it is trying to prevent the unlikely possibility of two newly computed hashes from being identical before being created by the delegator.  The EntityModel has the emailAddress as the primary key so I presume a duplicate hash would be tolerated by the DB - thereby needing synch.  If this is indeed a hazard, they why aren't synchs like this much more common throughout the codebase?  What makes this code special such that synch like this is needed because the code needs protection beyond that which OFBIZ already provides for entities and services?  

Why is ContactMechServices.class chosen as the Lock object?
The whole Class Object is being used as a lock.  What shared state of the ContactMechServices is being protected?  Or what else within ContactMechServices could be going on that might interfere with this method?  The only data Objects in the class are immutable Strings, everything else are static methods.

Why is the lock on ContactMechServices.class held for this entire block?
Operations like the hash computation which results aren't shared and are local to the executing thread wouldn't need to be included in the synch block.  I know that one is supposed to hold locks for only as long as necessary to protect shared state.  In order to prevent potential deadlocks, one also shouldn't hold a lock while making calls to "alien" methods which might acquire locks of their own.  

Just trying to understand the reasoning behind why this type of synch is only sometimes used in OFBIZ.  To me it isn't clear what is trying to be accomplished with this kind of synch and there are no comments.  

Thank you everyone!

Brad

[Adrian Crum]

Brad,

Andrew Zeneski is the author of that code, so only he can tell you why
the synchronization code is there. In the meantime, I can share some
thoughts on other instances of synchronization code in OFBiz.

In most cases synchronization code is used to control multi-threaded
access to an object's static member variable. There are many examples of
this in the factory classes. The lock is placed on the member variable
if it is a non-null object. In those cases where the code has to create
the shared object, the lock is placed on the class (you can't put a lock
on a null variable).

Looking over the code you mentioned, I can't see a reason for
synchronization either. I'm looking forward to finding out what it is.

-Adrian

Brad Plies wrote:

> While reviewing how OFBIZ handled email verifications I observed the
> synchronized code below.  I do not understand why it is synchronized and
> finding the answers as to why will greatly help me better understand the
> inner workings of OFBIZ.
>
>
> in ContactMechServices.createEmailAddressVerification
> Line 1057 - 1082
>
>         synchronized(ContactMechServices.class) {
>             while(true){
>                 Long random = secureRandom.nextLong();
>                 verifyHash = HashCrypt.getDigestHash(Long.toString(random),
> "MD5");
>                 List<GenericValue> emailAddVerifications = null;
>                 try {
>                     emailAddVerifications =
> delegator.findByAnd("EmailAddressVerification", UtilMisc.toMap("verifyHash",
> verifyHash));
>                 } catch (GenericEntityException e) {
>                     Debug.logError(e.getMessage(), module);
>                     return ServiceUtil.returnError(e.getMessage());
>                 }
>                 if(UtilValidate.isEmpty(emailAddVerifications)) {
>                     GenericValue emailAddressVerification =
> delegator.makeValue("EmailAddressVerification");
>                     emailAddressVerification.set("emailAddress",
> emailAddress);
>                     emailAddressVerification.set("verifyHash", verifyHash);
>                     emailAddressVerification.set("expireDate", expireDate);
>                     try {
>                         delegator.create(emailAddressVerification);
>                     } catch (GenericEntityException e) {
>                         Debug.logError(e.getMessage(),module);
>                         return ServiceUtil.returnError(e.getMessage());
>                     }
>                     break;
>                 }
>             }
>         }
>
>
> Why is there synchronization here?  
> I can only find synch like this in 2 other places in OFBIZ applications.
> There are about 13 instances of this type of synch in the OFBIZ framework
> (in some Utils & Crypt).
>
> What is the shared state being protected?  Or what code must be guaraunteed
> to be running in no more than one thread at a time?
> I can only guess it is trying to prevent the unlikely possibility of two
> newly computed hashes from being identical before being created by the
> delegator.  The EntityModel has the emailAddress as the primary key so I
> presume a duplicate hash would be tolerated by the DB - thereby needing
> synch.  If this is indeed a hazard, they why aren't synchs like this much
> more common throughout the codebase?  What makes this code special such that
> synch like this is needed because the code needs protection beyond that
> which OFBIZ already provides for entities and services?  
>
> Why is ContactMechServices.class chosen as the Lock object?
> The whole Class Object is being used as a lock.  What shared state of the
> ContactMechServices is being protected?  Or what else within
> ContactMechServices could be going on that might interfere with this method?
> The only data Objects in the class are immutable Strings, everything else
> are static methods.
>
> Why is the lock on ContactMechServices.class held for this entire block?
> Operations like the hash computation which results aren't shared and are
> local to the executing thread wouldn't need to be included in the synch
> block.  I know that one is supposed to hold locks for only as long as
> necessary to protect shared state.  In order to prevent potential deadlocks,
> one also shouldn't hold a lock while making calls to "alien" methods which
> might acquire locks of their own.  
>
> Just trying to understand the reasoning behind why this type of synch is
> only sometimes used in OFBIZ.  To me it isn't clear what is trying to be
> accomplished with this kind of synch and there are no comments.  
>
> Thank you everyone!
>
> Brad