Comment 1 David McKnight 2012-01-12 09:49:17 EST

Created attachment 209382 [details]
patch to provide thread safety for cachedRemoteFiles

Comment 2 David McKnight 2012-01-12 09:52:09 EST

I've committed the fix to the HEAD stream.

Comment 3 Xuan Chen 2012-01-12 11:08:33 EST

The fix looks good.  Thanks.

Comment 4 Martin Oberhuber 2012-01-31 08:03:04 EST

The _cachedRemoteFiles field is protected, and therefore it is API. When you make all access to that field synchronized, you should add a Javadoc comment to the field indicating that any access to it must be synchronized -- or extenders of the class could ruin your thread-safety.

Besides adding the Javadoc which I'd expect, there's a 2nd thing we can learn here... usually, instead off adding your synchronized blocks everywhere, I would have done this:

protected HashMap _cachedRemoteFiles = Collections.synchronizedMap(new HashMap());

but, unfortunately, your API exports type "HashMap" and not type "Map" so this is not possible without breaking API. What we learn from this is that API classes should never be concrete classes (like HashMap) but generic classes or interfaces (like Map).

Comment 5 David McKnight 2012-02-01 11:26:43 EST

Created attachment 210380 [details]
amendment to patch with javadoc comment about field synchronization

Comment 6 David McKnight 2012-02-01 11:29:26 EST

(In reply to comment #4)
> The _cachedRemoteFiles field is protected, and therefore it is API. When you
> make all access to that field synchronized, you should add a Javadoc comment to
> the field indicating that any access to it must be synchronized -- or extenders
> of the class could ruin your thread-safety.

I've added a javadoc comment for this.

> 
> Besides adding the Javadoc which I'd expect, there's a 2nd thing we can learn
> here... usually, instead off adding your synchronized blocks everywhere, I
> would have done this:
> 
> protected HashMap _cachedRemoteFiles = Collections.synchronizedMap(new
> HashMap());
> 
> but, unfortunately, your API exports type "HashMap" and not type "Map" so this
> is not possible without breaking API. What we learn from this is that API
> classes should never be concrete classes (like HashMap) but generic classes or
> interfaces (like Map).

Good point and certainly a good lesson to go forward with.

Comment 7 David McKnight 2012-02-01 11:30:06 EST

I've committed the update to the HEAD stream.

Comment 8 Martin Oberhuber 2012-02-01 11:36:15 EST

Thanks for the Javadoc blurb but I think it's not ideal yet:

1. You may want to discourage using the Field altoghether
   (with an @noreference tag) as well as set an @deprecated tag in order
   to enable us make the field non-API in the future
2. The first sentence in any Javadoc comment should be a "summary" kind of
   description of the item
3. If you require synchronizing, you need to specify what object users should
   synchronize on (the hasmap itself, in this case, which is not exactly 
   obvious). A small example code snippet would make sense here.

In fact, I think the code would benefit if you add an
      RemoteFileSubSystem#clearRemoteFileCache()
protected method, and use that new method in FileServiceSubSystem (such that
you don't have to have the field protected any more).

Comment 9 David McKnight 2012-02-01 11:53:38 EST

Created attachment 210389 [details]
amendment to patch with javadoc comment about field synchronization

How's this updated comment?  I can't add the @noreference right now because the compiler will complain.

Comment 10 David McKnight 2012-02-01 13:36:54 EST

Created attachment 210392 [details]
amendment to patch with new clearRemoteFileCache() api

Comment 11 David McKnight 2012-02-01 13:51:06 EST

Martin, with the patch amendment that deprecates _cachedRemoteFiles, do you thing it's still necessary to show the code example?

Comment 12 Martin Oberhuber 2012-05-07 15:06:32 EDT

I think we can mark this fixed, right ?

Comment 13 David McKnight 2012-05-07 15:50:56 EDT

(In reply to comment #12)
> I think we can mark this fixed, right ?

Yes, this is fixed.