From: Shawn O. Pearce <spearce@spearce.org>
Date: Fri, 11 Jul 2008 04:28:22 GMT
Subject: Re: [EGIT PATCH 4/9] Add a method to listen to changes in any repository
Message-ID: <20080711042822.GC32633@spearce.org>
In-Reply-To: <1215729651-26781-5-git-send-email-robin.rosenberg@dewire.com>

Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> diff --git a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> index 6f78652..dfa3045 100644
> --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> @@ -95,6 +95,7 @@ public class Repository {
>  	private GitIndex index;
>  
>  	private List<RepositoryListener> listeners = new Vector<RepositoryListener>(); // thread safe
> +	static private List<RepositoryListener> allListeners = new Vector<RepositoryListener>(); // thread safe
...
>  	void fireRefsMaybeChanged() {
>  		if (refs.lastRefModification != refs.lastNotifiedRefModification) {
>  			refs.lastNotifiedRefModification = refs.lastRefModification;
>  			final RefsChangedEvent event = new RefsChangedEvent(this);
> -			for (final RepositoryListener l :
> -				listeners.toArray(new RepositoryListener[listeners.size()])) {
> +			List<RepositoryListener> all = new ArrayList<RepositoryListener>(
> +					listeners);
> +			all.addAll(allListeners);
> +			for (final RepositoryListener l : all) {
>  				l.refsChanged(event);

I don't think this pattern is thread-safe like you think it is.
Adding (or removing) an allListener while you are trying
to copy the allListener collection for delivery can cause a
ConcurrentModificationException.

The preimage here is not even correct because toArray locks the
vector and will grow the input array if it was too small, but it
nulls out the later entries if the array was too large.  Thus you
can NPE inside of the fire loop.

The only safe way to do this is to lock the collection while you copy
it into an array or list, then later iterate that to do the delivery.

Both of the fire implemenations in this patch have this issue.  :-|

-- 
Shawn.