You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Brad Plies <bp...@bulliondirect.com> on 2008/09/04 01:52:27 UTC
Needless synchronization?
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
--
View this message in context: http://www.nabble.com/Needless-synchronization--tp19300547p19300547.html
Sent from the OFBiz - Dev mailing list archive at Nabble.com.
Re: Needless synchronization?
Posted by Adrian Crum <ad...@hlmksw.com>.
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