Stefan: Your suggested changes on PropertyHelper sound reasonable; I'm playing with them now.
-Matt --- Stefan Bodewig <[EMAIL PROTECTED]> wrote: > Hi all, > > while looking into > https://issues.apache.org/bugzilla/show_bug.cgi?id=45194 > I realized > that we are currently pretty inconsistent with our > usage of locking in > Project. > > Right now we have: > > * locking on the Project instance for the logging > subsystem (add or > remove a BuildListener and the actual logging) > > * locking on the PropertyHelper instance on all > public PropertyHelper > methods > > * locking on the Project instance when associating a > task with the > current thread - but not when reading it > > * locking of the references table when setting a > reference, but not > when reading it > > The first two cause the race condition noted in the > bug report since > the methods in PropertyHelper may log stuff while > another thread is > active logging something and its currently executing > BuildListener > tries to read property. > > There are two places where we potentially call > external (non-Ant) code > inside of a synchronized section: while logging (a > BuildListener) and > while accessing properties (a PropertySetter or > PropertyEvaluator). > Either should be avoided, but I'm not sure we can. > > We use locking for the log system for two reasons: > locking the > collection of listeners and setting a flag that > allows us to detect > recursive invocations of the logging system in order > to avoid infinite > loops (a BuildListener writing to System.out/err). > > I suggest the following changes: > > * add locking to getThreadTask, but don't use the > project instance but > rather something like the threadTasks table (and > use that in > registerTreadTask, of course). > > * remove the locking of references completely. > Given we hand out full > control via Project.getReferences to whoever is > there, locking the > write access to make the "do I need to print a > warning? Add the > reference" operation atomic seems pretty useless > to me. > > * lock the listener collection in the add/remove > listener methods, > in fireMessageLogged lock the listeners, clone > them, give up the > lock, work on the clone > > * turn the loggingMessage flag into a ThreadLocal, > don't lock for it > at all. > > The last two changes allow BuildListeners to be > executed without > holding any locks - this should avoid the deadlock > in issue 45194. > > * I'm not completely sure what the reasons for > locking in > PropertyHelper are and whether: > > * lock the delegates collections in add and clone > it in all the > other methods > > * invoke the delegate without any locks > > * lock on the property helper instance after all > delegates have been > invoked > > would meet all the needs. Matt? > > Stefan > > --------------------------------------------------------------------- > To unsubscribe, e-mail: > [EMAIL PROTECTED] > For additional commands, e-mail: > [EMAIL PROTECTED] > > --------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
