You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@abdera.apache.org by jm...@apache.org on 2007/04/25 16:46:18 UTC
svn commit: r532373 -
/incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java
Author: jmsnell
Date: Wed Apr 25 07:46:17 2007
New Revision: 532373
URL: http://svn.apache.org/viewvc?view=rev&rev=532373
Log:
Second attempt at improving thread safety on ExtensionFactoryMap. Wrap the weakhashmap with a synchronized map and synchronize the
iterations over the factories list
Modified:
incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java
Modified: incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java
URL: http://svn.apache.org/viewvc/incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java?view=diff&rev=532373&r1=532372&r2=532373
==============================================================================
--- incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java (original)
+++ incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java Wed Apr 25 07:46:17 2007
@@ -18,6 +18,7 @@
package org.apache.abdera.factory;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.WeakHashMap;
@@ -34,8 +35,9 @@
private final Map<Element,Element> wrappers;
public ExtensionFactoryMap(List<ExtensionFactory> factories) {
- this.factories = factories;
- this.wrappers = new WeakHashMap<Element,Element>();
+ this.factories = factories;;
+ this.wrappers = Collections.synchronizedMap(
+ new WeakHashMap<Element,Element>());
}
@SuppressWarnings("unchecked")
@@ -43,11 +45,13 @@
if (internal == null) return null;
T t = (T)wrappers.get(internal);
if (t == null) {
- for (ExtensionFactory factory : factories) {
- t = (T) factory.getElementWrapper(internal);
- if (t != null && t != internal) {
- setElementWrapper(internal,t);
- return t;
+ synchronized(factories) {
+ for (ExtensionFactory factory : factories) {
+ t = (T) factory.getElementWrapper(internal);
+ if (t != null && t != internal) {
+ setElementWrapper(internal,t);
+ return t;
+ }
}
}
t = (T) internal;
@@ -55,21 +59,25 @@
return (t != null) ? t : (T)internal;
}
- public synchronized void setElementWrapper(Element internal, Element wrapper) {
+ public void setElementWrapper(Element internal, Element wrapper) {
wrappers.put(internal, wrapper);
}
public List<String> getNamespaces() {
List<String> ns = new ArrayList<String>();
- for (ExtensionFactory factory : factories) {
- ns.addAll(factory.getNamespaces());
+ synchronized(factories) {
+ for (ExtensionFactory factory : factories) {
+ ns.addAll(factory.getNamespaces());
+ }
}
return ns;
}
public boolean handlesNamespace(String namespace) {
- for (ExtensionFactory factory : factories) {
- if (factory.handlesNamespace(namespace)) return true;
+ synchronized(factories) {
+ for (ExtensionFactory factory : factories) {
+ if (factory.handlesNamespace(namespace)) return true;
+ }
}
return false;
}
Re: svn commit: r532373 - /incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java
Posted by James M Snell <ja...@gmail.com>.
Yep. I've just made a change that the factories list is completely
enclosed within ExtensionFactoryMap. All access to it is via methods on
the map class. It will never be directly exposed to the FOMFactory.
I've also wrapped the copied list using Collections.synchronizedList()
and sync'd all iterations over it. That should be enough.
- James
Garrett Rooney wrote:
> On 4/25/07, James M Snell <ja...@gmail.com> wrote:
>> Wait, I take that back... each Factory impl is already getting it's own
>> copy of the Factories list. So long as we serialize access to it, I
>> don't think it's going to be much of a problem.
>
> I'm not sure I follow here. Maybe this means the constructor doesn't
> need to worry about copying the list, but it seems extremely unwise to
> have the accessor method simply return a reference to it, as that
> means the caller needs to know how to serialize on it in order to
> safely do anything with it.
>
> -garrett
>
Re: svn commit: r532373 - /incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 4/25/07, James M Snell <ja...@gmail.com> wrote:
> Wait, I take that back... each Factory impl is already getting it's own
> copy of the Factories list. So long as we serialize access to it, I
> don't think it's going to be much of a problem.
I'm not sure I follow here. Maybe this means the constructor doesn't
need to worry about copying the list, but it seems extremely unwise to
have the accessor method simply return a reference to it, as that
means the caller needs to know how to serialize on it in order to
safely do anything with it.
-garrett
Re: svn commit: r532373 - /incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java
Posted by James M Snell <ja...@gmail.com>.
Wait, I take that back... each Factory impl is already getting it's own
copy of the Factories list. So long as we serialize access to it, I
don't think it's going to be much of a problem.
- James
James M Snell wrote:
> Yeah, I was looking at the same thing while I was adding this bit of
> code in. I was trying to decide what else, if anything I should do with it.
>
> The factories map is created initially by the AbderaConfiguration and
> passed into the Factory implementation on init. Each FOMFactory
> instance has only a single ExtensionFactoryMap instance. Assuming that
> most users will end up using the default Factory implementation, I
> wouldn't see a problem in creating a copy for each Factory instance.
>
> - James
>
> Garrett Rooney wrote:
>> On 4/25/07, jmsnell@apache.org <jm...@apache.org> wrote:
>>> Author: jmsnell
>>> Date: Wed Apr 25 07:46:17 2007
>>> New Revision: 532373
>>>
>>> URL: http://svn.apache.org/viewvc?view=rev&rev=532373
>>> Log:
>>> Second attempt at improving thread safety on ExtensionFactoryMap.
>>> Wrap the weakhashmap with a synchronized map and synchronize the
>>> iterations over the factories list
>> Other potential issues in this code WRT thread safety:
>>
>> We don't make a defensive copy of the factories list when we're
>> created, so potentially someone else can synchronize on it, resulting
>> in deadlock. Alternatively it could be modified without
>> synchronization after we're created, defeating the purpose of the
>> other locking we do. Similarly, we return a reference to it in
>> getFactories(), with identical potential badness.
>>
>> The first case can be dealt with via documentation if we don't want to
>> make a copy, but the second really seems like it should be dealt with
>> by making a copy of the list before returning it.
>>
>> -garrett
>>
>
Re: svn commit: r532373 - /incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java
Posted by James M Snell <ja...@gmail.com>.
Yeah, I was looking at the same thing while I was adding this bit of
code in. I was trying to decide what else, if anything I should do with it.
The factories map is created initially by the AbderaConfiguration and
passed into the Factory implementation on init. Each FOMFactory
instance has only a single ExtensionFactoryMap instance. Assuming that
most users will end up using the default Factory implementation, I
wouldn't see a problem in creating a copy for each Factory instance.
- James
Garrett Rooney wrote:
> On 4/25/07, jmsnell@apache.org <jm...@apache.org> wrote:
>> Author: jmsnell
>> Date: Wed Apr 25 07:46:17 2007
>> New Revision: 532373
>>
>> URL: http://svn.apache.org/viewvc?view=rev&rev=532373
>> Log:
>> Second attempt at improving thread safety on ExtensionFactoryMap.
>> Wrap the weakhashmap with a synchronized map and synchronize the
>> iterations over the factories list
>
> Other potential issues in this code WRT thread safety:
>
> We don't make a defensive copy of the factories list when we're
> created, so potentially someone else can synchronize on it, resulting
> in deadlock. Alternatively it could be modified without
> synchronization after we're created, defeating the purpose of the
> other locking we do. Similarly, we return a reference to it in
> getFactories(), with identical potential badness.
>
> The first case can be dealt with via documentation if we don't want to
> make a copy, but the second really seems like it should be dealt with
> by making a copy of the list before returning it.
>
> -garrett
>
Re: svn commit: r532373 - /incubator/abdera/java/trunk/core/src/main/java/org/apache/abdera/factory/ExtensionFactoryMap.java
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
On 4/25/07, jmsnell@apache.org <jm...@apache.org> wrote:
> Author: jmsnell
> Date: Wed Apr 25 07:46:17 2007
> New Revision: 532373
>
> URL: http://svn.apache.org/viewvc?view=rev&rev=532373
> Log:
> Second attempt at improving thread safety on ExtensionFactoryMap. Wrap the weakhashmap with a synchronized map and synchronize the
> iterations over the factories list
Other potential issues in this code WRT thread safety:
We don't make a defensive copy of the factories list when we're
created, so potentially someone else can synchronize on it, resulting
in deadlock. Alternatively it could be modified without
synchronization after we're created, defeating the purpose of the
other locking we do. Similarly, we return a reference to it in
getFactories(), with identical potential badness.
The first case can be dealt with via documentation if we don't want to
make a copy, but the second really seems like it should be dealt with
by making a copy of the list before returning it.
-garrett