Re: [EGIT PATCH 2/5] quickdiff - Don't add GitDocument as repository listener more than once

[Robin Rosenberg <robin.rosenberg.lists@dewire.com>]

söndag 05 april 2009 22:24:00 skrev "Shawn O. Pearce" <spearce@spearce.org>:
> Robin Rosenberg <robin.rosenberg@dewire.com> wrote:
> > Doing so was very costly and led to sessions being locked
> > for a long time while refreshing the reference document used
> > by Eclipse's quickdiff feature.
> ...
> > --- a/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> > +++ b/org.spearce.jgit/src/org/spearce/jgit/lib/Repository.java
> > @@ -1132,6 +1132,7 @@ public File getWorkDir() {
> >  	 * @param l
> >  	 */
> >  	public void addRepositoryChangedListener(final RepositoryListener l) {
> > +		assert !listeners.contains(l);
> >  		listeners.add(l);
> >  	}
> >  
> > @@ -1150,6 +1151,7 @@ public void removeRepositoryChangedListener(final RepositoryListener l) {
> >  	 * @param l
> >  	 */
> >  	public static void addAnyRepositoryChangedListener(final RepositoryListener l) {
> > +		assert !allListeners.contains(l);
> >  		allListeners.add(l);
> >  	}
> 
> That's a race condition.  The two collections are Vectors so another
> thread can add the listener between your assertion point and the
> add call.

That would be another programming error, to add the same listener from different threads. Hopefully it would be caught on the next run. Adding synchronized might be too much since I cannot disable it by disabling assert.

> Also, if this really is considered to be very bad behavior on the
> part of the application, maybe these should be real tests with
> exceptions thrown, so they can't be disabled when assertions are
> turned off in the JRE?

It's usually not terribly bad, but what happens isn't well defined. If it happens it's a programming error. Especially now that the API bans it.

Asserts are for detecting programming errors during program test. If the tests are good enough there will never be a reason for this assert to fail and so it won't be necessary in deployed code. We could probably have lots more of these
declarations of assumptions, most of which would be less obvious than this simple test.

In particular asserts are for detecting bad conditions the programmer controls (or should control). Anything that can be the result of user actions or depending on the runtime environment should have non assert tests.

-- robin