[
https://issues.apache.org/jira/browse/OPENJPA-826?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12656263#action_12656263
]
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;
+
lock();
try {
- assertOpen();
- if (_resultClass != null || _compiled != null || _query == null
- || _broker == null)
- return _resultClass;
-
// check again after compilation; maybe encoded in string
compileForCompilation();
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 safe:
private volatile Class _resultClass;
...
public Class getResultType() {
assertOpen();
if (_resultClass == null) {
lock();
if (_resultClass == null) {
compileForCompilation();
}
unlock();
}
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: https://issues.apache.org/jira/browse/OPENJPA-826
> 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, QueryImpl.getParameterDeclaration
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
