Re: [EGIT PATCH 02/11] Use Set instead of array to keep track of change listeners

From: Shawn O. Pearce <spearce@spearce.org>
Date: Feb 5, 2009, 15:48 UTC
Message-ID: <20090205154815.GG26880@spearce.org>
In-Reply-To: <1233795618-20249-3-git-send-email-torarnv@gmail.com>

Tor Arne Vestbø <torarnv@gmail.com> wrote:

Show 19 quoted lines

> +	public static synchronized void removeRepositoryChangeListener(
> +			final RepositoryChangeListener objectThatCares) {
> +		repositoryChangeListeners.remove(objectThatCares);
>  	}
>  
>  	/**
> @@ -131,13 +133,8 @@ public static synchronized void addRepositoryChangeListener(
>  	 *            the repository which has had changes occur within it.
>  	 */
>  	static void fireRepositoryChanged(final RepositoryMapping which) {
> -		final RepositoryChangeListener[] e = getRepositoryChangeListeners();
> -		for (int k = e.length - 1; k >= 0; k--)
> -			e[k].repositoryChanged(which);
> -	}
> -
> -	private static synchronized RepositoryChangeListener[] getRepositoryChangeListeners() {
> -		return repositoryChangeListeners;
> +		for (RepositoryChangeListener listener : repositoryChangeListeners)
> +			listener.repositoryChanged(which);

See anything wrong here, like that the Set can be modified while GitProjectData's class lock is held, but its being read here without any locking?

The array trick worked before because we always made a copy anytime the array was modified. So we could safely return the array to the caller and let the caller iterate it unlocked; we just had to read the current array using a synchronized method to ensure we had a stable read.

You'll need to copy the Set somehow while inside of a synchronized method, then return the copy to the fireRepositoryChanged() method so it can iterate the copy to fire the events.

-- 
Shawn.