[jbossts-issues] [JBoss JIRA] Commented: (JBTM-365) addSynchronization calls during beforeCompletion are problematic

[ http://jira.jboss.com/jira/browse/JBTM-365?page=comments#action_12417823 ] Andrew Dinn commented on JBTM-365: ---------------------------------- Pavel, yes this is planned. When the field synchs is being initialized in addSynchronization we synchronize on _this_ to make sure that only one thread assigns the field. We use _this_ because _this_ is the object being modified. At this point synchs may not even identify an object and we cannot synchronize on null :-) When we are doing beforeCompletion processing we use syncLock to guard the test of field beforeCalled. This field is set to true by the first thread to get past the synchronization point ensuring that no other thread tries to perform beforeCompletion processing. It is quite deliberately not used to control modifications to the list in synchs. Its only job is to make the branch from if (beforeCalled) single-threaded. We could synchronize on _this_ in beforeCompletion but that would stop other threads using _this_ to synchronize. Since we don't care what happens to any other data in _this_ other than the field beforeCalled a separate lock just for this field is a good idea. beforeCompletion cannot synchronize on the list in field synchs because it must be possible for other threads to call addSynch while beforeCompletion is running inside the while loop. Also, it might be the case that synchs is null and we cannot synchronize on null. Also, note that addSynch inserts into synchs vy calling synchs.add() and add() is a synchronized operation. So, if we were to use the list in synchs to guard the branch instead of syncLock this would stop other threads from adding synchronizations which breaks the spec. And finally . . . this is also why beforeCompletion temporarily locks the list held in synchs when comparing the length and, possibly, recopying it. If it did not lock synchs then another thread might be in the middle of a call to add() when it called copy(). Calling copy on a list while it is in the middle of being processed was what led to this issue being raised. I think there is still a race here. The while loop in beforeCompletion calls _synchs.size() to decide if it needs to process any more synchronizations. A concurrent thread may try to perform an add() just as this test is performed (or just after). Its synch may get added but will not get processed. Strictly, there ought to be a state change here so that the add fails and gets an AR_REJECTED error. However, the only code which suffers here is dumb code which does not ensure that it knows when it is safe to add a synch and when not to. So, I'm not losing nay sleep over it. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.jboss.com/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira