Stefan:
Your suggested changes on PropertyHelper sound
reasonable; I'm playing with them now.
-Matt
--- Stefan Bodewig <bodewig@apache.org> 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:
> dev-unsubscribe@ant.apache.org
> For additional commands, e-mail:
> dev-help@ant.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
|