After writing a few more auxiliary strategies, like one that can turn annotated class files into an XML configuration file with the equivalent annotations, I’ve created a branch for DrJava now and am adding my thread checking annotations.
So far, I’ve just searched for the string event thread” and put a @OnlyRunBy(@ThreadDesc(eventThread=true)) annotation in front of all the method that claimed they “should only be run in the event thread”. I haven’t performed any deeper analysis on whether that’s actually necessary or not. This pretty random process has lead me to insert 77 annotations.
There were three subtyping warnings that were produced:
edu.rice.cs.drjava.model.DefaultGlobalModel$ConcreteOpenDefDoc.runMain()V
has @OnlyRunBy event thread
but edu.rice.cs.drjava.model.AbstractGlobalModel$ConcreteOpenDefDoc.runMain()V
does not
edu.rice.cs.drjava.ui.MainFrame.pack()V has @OnlyRunBy event thread
but java.awt.Window.pack()V does not
edu.rice.cs.util.docnavigation.JListSortNavigator.addDocument
(Ledu/rice/cs/util/docnavigation/INavigatorItem;)V
has @OnlyRunBy event thread
but edu.rice.cs.util.docnavigation.JListNavigator.addDocument
(Ledu/rice/cs/util/docnavigation/INavigatorItem;)V
does not
The first one is a semi-valid concern: The supermethod should be annotated as well; since there are no other subclasses in this case, though, it did not really matter. Except for the comment that this method “is usually executed in the event thread”, I also did not find a reason for it. I added the annotation anyway.
The second warning is bogus, I think. To me it seems like calling pack is always safe. Here’s the code in question:
/** Ensures that pack() is run in the event thread. Only used in test code */
@OnlyRunBy(@ThreadDesc(eventThread=true))
public void pack() {
Utilities.invokeAndWait(new Runnable() { public void run() { packHelp(); } });
}
/** Helper method that provides access to super.pack() within the
* anonymous class new Runnable() {...} above */
private void packHelp() { super.pack(); }
I think it’s more an issue of making sure that the “packing” has already occurred when the code continues rather than making sure it happens in the event thread. I ended up removing that annotation.
The third one was just a case where I had missed to add the annotation. With these three changes made, there were no more subtyping warnings. So far, so good.
When I started an instrumented version of the drjava-15.jar file, I already got my first violation:
Thread Violation: OnlyRunBy
Current thread 'main', id 1, group 'main' did not match
the event thread
at edu.rice.cs.drjava.config.KeyStrokeOption. (KeyStrokeOption.java:-1)
at edu.rice.cs.drjava.config.OptionConstants. (OptionConstants.java:278)
at sun.misc.Unsafe.ensureClassInitialized (Unsafe.java:-2)
at sun.reflect.UnsafeFieldAccessorFactory.newFieldAccessor (UnsafeFieldAccessorFac
at sun.reflect.ReflectionFactory.newFieldAccessor (ReflectionFactory.java:122)
at java.lang.reflect.Field.acquireFieldAccessor (Field.java:917)
at java.lang.reflect.Field.getFieldAccessor (Field.java:898)
at java.lang.reflect.Field.get (Field.java:357)
at edu.rice.cs.drjava.config.OptionMapLoader. (OptionMapLoader.java:57)
at edu.rice.cs.drjava.config.SavableConfiguration.loadConfiguration (SavableConfig
at edu.rice.cs.drjava.config.FileConfiguration.loadConfiguration (FileConfiguratio
at edu.rice.cs.drjava.DrJava._initConfig (DrJava.java:432)
at edu.rice.cs.drjava.DrJava. (DrJava.java:117)
at edu.rice.cs.drjava.DrJavaRoot.main (DrJavaRoot.java:97)
The code for KeyStrokeOption states that only the event thread accesses the methods in this class:
/** Class representing all configuration options with values of type KeyStroke.
* This code should only run in the event thread, so no synchronization is necessary
* (or advisable).*/
@OnlyRunBy(@ThreadDesc(eventThread=true))
public class KeyStrokeOption extends Option
Obviously that isn’t true, the main thread also runs it. This is probably not a big issue, but it’s an error in the documentation. Other than that, DrJava has been holding up quite admirably so far. But right now I’m only checking against a few of our own constraints, and I haven’t run the unit tests yet.
I’ve also started looking at the Java API, and that, of course, is where it gets really interesting. I’ve found this comment in javax.swing.text.DefaultCaret, for example:
/** ...
* The repaint of the new caret location will occur on the event thread in any case,
* as calls to modelToView are only safe on the event thread.
I’m sure that I should mark javax.swing.text.DefaultCaret.repaintNewCaret() as event thread-only, but doesn’t the comment really imply that javax.swing.plaf.TextUI.modelToView should only be called in the event thread? I’ve marked those methods event thread-only too now, even though they seem to be used in many places without special concern.
