git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [EGIT PATCH 4/9] Add a method to listen to changes in any repository

From
Shawn O. Pearce <spearce@spearce.org>
Date
Jul 11, 2008, 04:28 UTC
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:
Show 9 quoted lines
> 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
...
Show 11 quoted lines
>  	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.
Previous: Robin RosenbergNext: Robin Rosenberg
Message 13 of 19 in “Repository change listeners”
  1. 0/9 Repository change listenersRobin Rosenberg, Jul 10, 2008
  2. 1/9 Create a listener structure for changes to refs and indexRobin Rosenberg, Jul 10, 2008
  3. 2/9 Cached modification times for symbolic refs tooRobin Rosenberg, Jul 10, 2008
  4. 3/9 Connect the history page to the refs update subscription mechanismRobin Rosenberg, Jul 10, 2008
  5. 4/9 Add a method to listen to changes in any repositoryRobin Rosenberg, Jul 10, 2008
  6. 5/9 Add a job to periodically scan for repository changesRobin Rosenberg, Jul 10, 2008
  7. 6/9 Change GitHistoryPage to listen on any repository.Robin Rosenberg, Jul 10, 2008
  8. 7/9 Add a job to refresh projects when the index changes.Robin Rosenberg, Jul 10, 2008
  9. 8/9 Make git dectected changes depend on the automatic refresh settingRobin Rosenberg, Jul 10, 2008
  10. 9/9 Attach the resource decorator to the repository change event mechanismRobin Rosenberg, Jul 10, 2008
  11. Shawn O. PearceJul 11, 2008
  12. 7/7 Add a job to refresh projects when the index changes.Robin Rosenberg, Jul 11, 2008
  13. Shawn O. PearceJul 11, 2008
  14. 4/4 Add a method to listen to changes in any repositoryRobin Rosenberg, Jul 11, 2008
  15. jgit (was: [PATCH 4/4] Add a method...)Andreas Ericsson, Jul 11, 2008
  16. Robin RosenbergJul 11, 2008
  17. Shawn O. PearceJul 11, 2008
  18. Create a listener structure for changes to refs and indexRobin Rosenberg, Jul 11, 2008
  19. Shawn O. PearceJul 11, 2008

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.