You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by Gilles Scokart <gs...@gmail.com> on 2008/08/20 20:31:02 UTC
Re: svn commit: r687358 - /ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java
I think there might be also a raise condition for the field
restrictedDefinitions. The update via addDataTypeDefinition is
guarded by the restrictedDefinitions object itself, but the read via
getRestrictedDefinitions is not (neither modification in
initSubProject).
But I'm not sure what a restricted datatype is, when they are defined,
and when they are used. Could it be done from multiple thread?
2008/8/20 <gs...@apache.org>:
> Author: gscokart
> Date: Wed Aug 20 07:40:03 2008
> New Revision: 687358
>
> URL: http://svn.apache.org/viewvc?rev=687358&view=rev
> Log:
> Thread safety fix in case a macrodef/presetdef is executed in parallel to other type related tasks (like TypeFound conditions or starting a subproject)
>
> Modified:
> ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java
>
> Modified: ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java
> URL: http://svn.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java?rev=687358&r1=687357&r2=687358&view=diff
> ==============================================================================
> --- ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java (original)
> +++ ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java Wed Aug 20 07:40:03 2008
> @@ -65,19 +65,19 @@
> private AntTypeTable antTypeTable;
>
> /** Map of tasks generated from antTypeTable */
> - private Hashtable taskClassDefinitions = new Hashtable();
> + private final Hashtable taskClassDefinitions = new Hashtable();
>
> /** flag to rebuild taskClassDefinitions */
> private boolean rebuildTaskClassDefinitions = true;
>
> /** Map of types generated from antTypeTable */
> - private Hashtable typeClassDefinitions = new Hashtable();
> + private final Hashtable typeClassDefinitions = new Hashtable();
>
> /** flag to rebuild typeClassDefinitions */
> private boolean rebuildTypeClassDefinitions = true;
>
> /** Set of namespaces that have been checked for antlibs */
> - private Set checkedNamespaces = new HashSet();
> + private final HashSet checkedNamespaces = new HashSet();
>
> /**
> * Stack of antlib contexts used to resolve definitions while
> @@ -193,6 +193,13 @@
> }
>
> /**
> + * @return A copy of the CheckedNamespace.
> + */
> + private synchronized Set getCheckedNamespace() {
> + return (Set) checkedNamespaces.clone();
> + }
> +
> + /**
> * Used with creating child projects. Each child
> * project inherits the component definitions
> * from its parent.
> @@ -200,14 +207,17 @@
> */
> public void initSubProject(ComponentHelper helper) {
> // add the types of the parent project
> - AntTypeTable typeTable = helper.antTypeTable;
> - for (Iterator i = typeTable.values().iterator(); i.hasNext();) {
> - AntTypeDefinition def = (AntTypeDefinition) i.next();
> - antTypeTable.put(def.getName(), def);
> + AntTypeTable typeTable = (AntTypeTable) helper.antTypeTable.clone();
> + synchronized (antTypeTable) {
> + for (Iterator i = typeTable.values().iterator(); i.hasNext();) {
> + AntTypeDefinition def = (AntTypeDefinition) i.next();
> + antTypeTable.put(def.getName(), def);
> + }
> }
> // add the parsed namespaces of the parent project
> - for (Iterator i = helper.checkedNamespaces.iterator(); i.hasNext();) {
> - checkedNamespaces.add(i.next());
> + Set inheritedCheckedNamespace = helper.getCheckedNamespace();
> + synchronized (this) {
> + checkedNamespaces.addAll(inheritedCheckedNamespace);
> }
>
> // Add the restricted definitions
> @@ -581,12 +591,14 @@
> // but this is for logging only...
> Class elementClass = o.getClass();
> String elementClassname = elementClass.getName();
> - for (Iterator i = antTypeTable.values().iterator(); i.hasNext();) {
> - AntTypeDefinition def = (AntTypeDefinition) i.next();
> - if (elementClassname.equals(def.getClassName())
> - && (elementClass == def.getExposedClass(project))) {
> - String name = def.getName();
> - return brief ? name : "The <" + name + "> type";
> + synchronized (antTypeTable) {
> + for (Iterator i = antTypeTable.values().iterator(); i.hasNext();) {
> + AntTypeDefinition def = (AntTypeDefinition) i.next();
> + if (elementClassname.equals(def.getClassName())
> + && (elementClass == def.getExposedClass(project))) {
> + String name = def.getName();
> + return brief ? name : "The <" + name + "> type";
> + }
> }
> }
> return getUnmappedElementName(o.getClass(), brief);
> @@ -1071,7 +1083,7 @@
> return def == null ? null : def.getExposedClass(project);
> }
>
> - public boolean contains(Object clazz) {
> + public synchronized boolean contains(Object clazz) {
> boolean found = false;
> if (clazz instanceof Class) {
> for (Iterator i = values().iterator(); i.hasNext() && !found;) {
> @@ -1091,7 +1103,7 @@
> * @param prefix prefix to match off
> * @return the (possibly empty) list of definitions
> */
> - public List/*<AntTypeDefinition>*/ findMatches(String prefix) {
> + public synchronized List/*<AntTypeDefinition>*/ findMatches(String prefix) {
> ArrayList matches = new ArrayList();
> for (Iterator i = values().iterator(); i.hasNext();) {
> AntTypeDefinition def = (AntTypeDefinition) (i.next());
>
>
>
--
Gilles Scokart
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: svn commit: r687358 - /ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java
Posted by Stefan Bodewig <bo...@apache.org>.
On Mon, 25 Aug 2008, Gilles Scokart <gs...@gmail.com> wrote:
> I also had to guard getRestrictedDefinitions, and the caller is
> supposed to guard the resulting List. Which is quiet anoying.
> Maybe the returned list should be a copy, and not the life instance.
> WDYT?
I'm not sure who calls getRestrictedDefinitions, is it called often?
If it is not performance critical in any way then it may be worth
creating a copy.
All in all I'd say this race condition is pretty much a theoretical
one since componentdef-ing inside of parallel doesn's sound like
something people would do regularly.
Stefan
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: svn commit: r687358 - /ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java
Posted by Gilles Scokart <gs...@gmail.com>.
There was indeed a possible raise condition for modification/reading
restrictedDefinitions.
The modification done in initSubProject was not really a problem
(normally only called in the initialization of a sub project, from a
single thread), but the one done in updateRestrictedDefinition was.
It apears that it may be possible to have the
updateRestrictedDefinition called asynchronously from inside a
parallel. This method will be called when a new componentDef is
defined. (See [1] for a description of a componentDef)
I also had to guard getRestrictedDefinitions, and the caller is
supposed to guard the resulting List. Which is quiet anoying.
Maybe the returned list should be a copy, and not the life instance.
WDYT?
[1] See https://issues.apache.org/bugzilla/show_bug.cgi?id=40511
2008/8/20 Gilles Scokart <gs...@gmail.com>:
> I think there might be also a raise condition for the field
> restrictedDefinitions. The update via addDataTypeDefinition is
> guarded by the restrictedDefinitions object itself, but the read via
> getRestrictedDefinitions is not (neither modification in
> initSubProject).
> But I'm not sure what a restricted datatype is, when they are defined,
> and when they are used. Could it be done from multiple thread?
>
>
>
> 2008/8/20 <gs...@apache.org>:
>> Author: gscokart
>> Date: Wed Aug 20 07:40:03 2008
>> New Revision: 687358
>>
>> URL: http://svn.apache.org/viewvc?rev=687358&view=rev
>> Log:
>> Thread safety fix in case a macrodef/presetdef is executed in parallel to other type related tasks (like TypeFound conditions or starting a subproject)
>>
>> Modified:
>> ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java
>>
>> Modified: ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java
>> URL: http://svn.apache.org/viewvc/ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java?rev=687358&r1=687357&r2=687358&view=diff
>> ==============================================================================
>> --- ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java (original)
>> +++ ant/core/trunk/src/main/org/apache/tools/ant/ComponentHelper.java Wed Aug 20 07:40:03 2008
>> @@ -65,19 +65,19 @@
>> private AntTypeTable antTypeTable;
>>
>> /** Map of tasks generated from antTypeTable */
>> - private Hashtable taskClassDefinitions = new Hashtable();
>> + private final Hashtable taskClassDefinitions = new Hashtable();
>>
>> /** flag to rebuild taskClassDefinitions */
>> private boolean rebuildTaskClassDefinitions = true;
>>
>> /** Map of types generated from antTypeTable */
>> - private Hashtable typeClassDefinitions = new Hashtable();
>> + private final Hashtable typeClassDefinitions = new Hashtable();
>>
>> /** flag to rebuild typeClassDefinitions */
>> private boolean rebuildTypeClassDefinitions = true;
>>
>> /** Set of namespaces that have been checked for antlibs */
>> - private Set checkedNamespaces = new HashSet();
>> + private final HashSet checkedNamespaces = new HashSet();
>>
>> /**
>> * Stack of antlib contexts used to resolve definitions while
>> @@ -193,6 +193,13 @@
>> }
>>
>> /**
>> + * @return A copy of the CheckedNamespace.
>> + */
>> + private synchronized Set getCheckedNamespace() {
>> + return (Set) checkedNamespaces.clone();
>> + }
>> +
>> + /**
>> * Used with creating child projects. Each child
>> * project inherits the component definitions
>> * from its parent.
>> @@ -200,14 +207,17 @@
>> */
>> public void initSubProject(ComponentHelper helper) {
>> // add the types of the parent project
>> - AntTypeTable typeTable = helper.antTypeTable;
>> - for (Iterator i = typeTable.values().iterator(); i.hasNext();) {
>> - AntTypeDefinition def = (AntTypeDefinition) i.next();
>> - antTypeTable.put(def.getName(), def);
>> + AntTypeTable typeTable = (AntTypeTable) helper.antTypeTable.clone();
>> + synchronized (antTypeTable) {
>> + for (Iterator i = typeTable.values().iterator(); i.hasNext();) {
>> + AntTypeDefinition def = (AntTypeDefinition) i.next();
>> + antTypeTable.put(def.getName(), def);
>> + }
>> }
>> // add the parsed namespaces of the parent project
>> - for (Iterator i = helper.checkedNamespaces.iterator(); i.hasNext();) {
>> - checkedNamespaces.add(i.next());
>> + Set inheritedCheckedNamespace = helper.getCheckedNamespace();
>> + synchronized (this) {
>> + checkedNamespaces.addAll(inheritedCheckedNamespace);
>> }
>>
>> // Add the restricted definitions
>> @@ -581,12 +591,14 @@
>> // but this is for logging only...
>> Class elementClass = o.getClass();
>> String elementClassname = elementClass.getName();
>> - for (Iterator i = antTypeTable.values().iterator(); i.hasNext();) {
>> - AntTypeDefinition def = (AntTypeDefinition) i.next();
>> - if (elementClassname.equals(def.getClassName())
>> - && (elementClass == def.getExposedClass(project))) {
>> - String name = def.getName();
>> - return brief ? name : "The <" + name + "> type";
>> + synchronized (antTypeTable) {
>> + for (Iterator i = antTypeTable.values().iterator(); i.hasNext();) {
>> + AntTypeDefinition def = (AntTypeDefinition) i.next();
>> + if (elementClassname.equals(def.getClassName())
>> + && (elementClass == def.getExposedClass(project))) {
>> + String name = def.getName();
>> + return brief ? name : "The <" + name + "> type";
>> + }
>> }
>> }
>> return getUnmappedElementName(o.getClass(), brief);
>> @@ -1071,7 +1083,7 @@
>> return def == null ? null : def.getExposedClass(project);
>> }
>>
>> - public boolean contains(Object clazz) {
>> + public synchronized boolean contains(Object clazz) {
>> boolean found = false;
>> if (clazz instanceof Class) {
>> for (Iterator i = values().iterator(); i.hasNext() && !found;) {
>> @@ -1091,7 +1103,7 @@
>> * @param prefix prefix to match off
>> * @return the (possibly empty) list of definitions
>> */
>> - public List/*<AntTypeDefinition>*/ findMatches(String prefix) {
>> + public synchronized List/*<AntTypeDefinition>*/ findMatches(String prefix) {
>> ArrayList matches = new ArrayList();
>> for (Iterator i = values().iterator(); i.hasNext();) {
>> AntTypeDefinition def = (AntTypeDefinition) (i.next());
>>
>>
>>
>
>
>
> --
> Gilles Scokart
>
--
Gilles Scokart
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org