openjpa-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "David Jencks (JIRA)" <>
Subject [jira] Commented: (OPENJPA-826) moving some code outside of lock boundaries, that don't need to be locked
Date Sat, 13 Dec 2008 08:06:44 GMT


David Jencks commented on OPENJPA-826:

These changes look to me like they introduce serious thread safety issues.  For instance with
this snippet:

     public Class getResultType() {

+    	assertOpen();

+    	if (_resultClass != null || _compiled != null || _query == null

+    			|| _broker == null)

+    		return _resultClass;



         try {

-            assertOpen();

-            if (_resultClass != null || _compiled != null || _query == null

-                || _broker == null)

-                return _resultClass;


             // check again after compilation; maybe encoded in string


             return _resultClass;

_resultClass must be volatile in order to assure that it is a completely constructed object,
and an arbitrarily large number of threads can get into the locked block and  call compileForCompilation
which I assume sets _resultClass.

You might be able to fix it with code something like this, assuming the assertions are thread

private volatile Class _resultClass;

     public Class getResultType() {
        if (_resultClass == null) {
            if (_resultClass == null) {
        return _resultClass;

(exception handling and some of the conditions left out).  This will not work unless the varlable
is volatile, as unlocked threads could then read a partially initialized object.  Google for
double-checked locking for more details.

> moving some code outside of lock boundaries, that don't need to be locked
> -------------------------------------------------------------------------
>                 Key: OPENJPA-826
>                 URL:
>             Project: OpenJPA
>          Issue Type: Improvement
>          Components: kernel
>    Affects Versions: 2.0.0
>            Reporter: Fernando
>         Attachments: locks-asserts.diff
> I am reviewing code because of slices concurrency/multithreaded issues, and I see that
some code in QueryImpl was too aggressive in their locks.  So I reviewed QueryImpl and BrokerImpl...
> Mainly: assertMethods do not need to be within the lock.
> Then I found a few methods that were doing precondition/shortcut checking, but all within
locks, so that seemed like a waste:
> 1) QueryImpl.setCandiateExtent; checks to see if value setting is same as current value
> 2) these places check to see if it needs to calculate the value of not:
> QueryImpl.isUnique, QueryImpl.setCandiateExtent, QueryImpl.getCandidateType, QueryImpl.getResultType,

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

View raw message