[ http://jira.jboss.com/jira/browse/JBTM-365?page=comments#action_12413238 ] Andrew Dinn commented on JBTM-365: ---------------------------------- "Asynch calls to ...addSynchronization during beforeCompletion" ??? Is there a really good reason for making asynch calls? If so then this raises issues for the asynchronous addSynchronization client as well as the implementation of beforeCompletion. Specifically, how can you ensure that the asynch calls occur before the end of beforeCompletion processing? Can you please provide more details of what is required here? -- 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

[ http://jira.jboss.com/jira/browse/JBTM-365?page=comments#action_12413493 ] Andrew Dinn commented on JBTM-365: ---------------------------------- My apologies for being so picky. I was asking merely to check that the problem was one caused by app code, not app server code. You are right that we need to fix this as app code may rely on winning a race rather than imposing thread synchronization to make calls to addSynch safe. We need to fail gracefully if app code loses this race and modifies the list while we are copying. If app server code is causing this to happen then the AS probably ought to be employing its own synchronization mechanism to ensure that it regulates adds from asynch threads once beforeCompletion processing has commenced -- if not then it cannot guarantee whether they will run or not. n.b. adds from AS-registered synchronizations are safe since the list is not being checked when the synch beforeCompletion method is being called. Not quite. The timeout code will change the transaction status causing it to avoid beforeCompletion processing and to reject addSynch calls with a rolled back exception. That said there is a synch bug raised against the status test/set code which might still allow this error to occur (see JBTM-3564 and JBTM-356). Also, there is a faint possibility that afterCompletion processing might start under a timeout while an addSynch is suspended in mid update so there are race conditions here to be dealt with. Anyway, the upshot is that we need to lock the synchronization set when we do the add in addSynchronization and also when we check its length and, maybe, update the copy in beforeCompletion. -- 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

[ http://jira.jboss.com/jira/browse/JBTM-365?page=all ] Work on JBTM-365 started by Andrew Dinn. -- 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

[ http://jira.jboss.com/jira/browse/JBTM-365?page=all ] Andrew Dinn closed JBTM-365. ---------------------------- Resolution: Done Fixed for 4.4 CR1 release by revision r20099 -- 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

[ 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

[ https://jira.jboss.org/jira/browse/JBTM-365?page=com.atlassian.jira.plugi... ] Jonathan Halliday commented on JBTM-365: ---------------------------------------- There is an additional problem with afterCompletion setting _syncs = null, which causes synchronized(_sync) blocks elsewhere to throw NullPointerException. Better to _syncs.clear() instead. This only matters with beforeCompletions that take so long the tx times out. -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira