[RFC][netx]: unsafe write to HashMap

[Dr Andrew John Hughes]

On 12:13 Tue 05 Apr     , Denis Lila wrote:
> Hi.
> 
> I think I found a concurrency problem in
> ResourceTracker.addResource. This method is not static.
> It is called from different threads, and it writes to
> downloadOptions which is a static HashMap. This is not
> safe. The patch uses a ConcurrentHashMap. That should
> be enough to fix it.
> 
> Ok to push?
> 
> Regards,
> Denis.

No, this isn't enough.  This piece of code:

    private URL findBestUrl(Resource resource) {
1.      DownloadOptions options = downloadOptions.get(resource);
2.      if (options == null) {
3.            options = new DownloadOptions(false, false);
    }

may be subject to a race from addResource.  Line 1 may retrieve null,
but that thread may then be interrupted before line 2 can be executed.
In the meantime, the resource could be added to downloadOptions and
this thread would still have the old stale value.

It's also odd code in that this new instance is created but not stored
in the map.  So it seems possible that many threads could call this
method, all get a return value of null and create new instances of
DownloadOptions.  The control flow in this class is not very clear,
but I presume the null check means that there is a possibility that
this method can be called before the resource is added in addResource.

I would suggest replacing those three lines with:

DownloadOptions options = downloadOptions.putIfAbsent(resource, new DownloadOptions(false,false));

This ensures, atomically, that any existing value is retrieved and stored
in options, but if one is not present, the default is added and returned.
With addResource using put instead of putIfAbsent, it should still override
a default added by this method.

This also means there's a reason to follow Andrew's suggestion, as
putIfAbsent is unique to ConcurrentHashMap.  In general, the interface
type (Map) should be used rather than the specific type to allow easy
transition between different implementations.  It prevents code using
implementation-specific methods which would make later implementation
changes harder e.g. if this class had been using HashMap directly, it
may have relied on HashMap-only methods.

> diff -r 62011060e7ea ChangeLog
> --- a/ChangeLog	Tue Apr 05 12:07:10 2011 -0400
> +++ b/ChangeLog	Tue Apr 05 12:12:33 2011 -0400
> @@ -1,3 +1,9 @@
> +2011-04-05  Denis Lila  <dlila at redhat.com>
> +
> +	* netx/net/sourceforge/jnlp/cache/ResourceTracker.java:
> +	Remove unused import, add import.
> +	(downloadOptions): Make ConcurrentHashMap.
> +
>  2011-04-05  Denis Lila  <dlila at redhat.com>
>  
>  	* plugin/icedteanp/IcedTeaNPPlugin.cc
> diff -r 62011060e7ea netx/net/sourceforge/jnlp/cache/ResourceTracker.java
> --- a/netx/net/sourceforge/jnlp/cache/ResourceTracker.java	Tue Apr 05 12:07:10 2011 -0400
> +++ b/netx/net/sourceforge/jnlp/cache/ResourceTracker.java	Tue Apr 05 12:12:33 2011 -0400
> @@ -29,9 +29,9 @@
>  import java.net.URL;
>  import java.net.URLConnection;
>  import java.util.ArrayList;
> -import java.util.HashMap;
>  import java.util.List;
>  import java.util.Map;
> +import java.util.concurrent.ConcurrentHashMap;
>  import java.util.jar.JarOutputStream;
>  import java.util.jar.Pack200;
>  import java.util.jar.Pack200.Unpacker;
> @@ -121,7 +121,7 @@
>      private static ArrayList<Resource> queue = new ArrayList<Resource>();
>  
>      private static Map<Resource, DownloadOptions> downloadOptions =
> -        new HashMap<Resource, DownloadOptions>();
> +        new ConcurrentHashMap<Resource, DownloadOptions>();
>  
>      /** resource trackers threads are working for (used for load balancing across multi-tracker downloads) */
>      private static ArrayList<ResourceTracker> active =


-- 
Andrew :)

Free Java Software Engineer
Red Hat, Inc. (http://www.redhat.com)

Support Free Java!
Contribute to GNU Classpath and IcedTea
http://www.gnu.org/software/classpath
http://icedtea.classpath.org
PGP Key: F5862A37 (https://keys.indymedia.org/)
Fingerprint = EA30 D855 D50F 90CD F54D  0698 0713 C3ED F586 2A37