Skip to content

Commit 844c5b5

Browse files
author
rcartwright
committed
This revision fixes a concurrency bug (unexpected deadline of the GUI)
that I introduced in my last two commits when I tried to improve _wrappedPosList synchronziation in DefinitionsDocument. I also continued to try elimnate the sprurious "Stepping ..." strings printed by the debugStepTimer. The following files were modified: M src/edu/rice/cs/drjava/model/cache/DocumentCache.java M src/edu/rice/cs/drjava/model/definitions/DefinitionsDocument.java M src/edu/rice/cs/drjava/model/repl/newjvm/InterpreterJVM.java M src/edu/rice/cs/drjava/model/AbstractGlobalModel.java M src/edu/rice/cs/drjava/ui/MainFrame.java M src/edu/rice/cs/util/docnavigation/AWTContainerNavigatorFactory.java M src/edu/rice/cs/util/docnavigation/JTreeSortNavigator.java M src/edu/rice/cs/util/JoinInputStream.java M src/edu/rice/cs/util/newjvm/AbstractSlaveJVM.java git-svn-id: file:///tmp/test-svn/trunk@5445 fe72c1cf-3628-48e9-8b72-1c46755d3cff
1 parent 68fac93 commit 844c5b5

File tree

9 files changed

+180
-118
lines changed

9 files changed

+180
-118
lines changed

‎drjava/src/edu/rice/cs/drjava/model/AbstractGlobalModel.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2807,7 +2807,7 @@ public synchronized void setFile(final File file) {
28072807
public void updateSyntaxHighlighting() {
28082808
// can't be called in AbstractGlobalModel.ConcreteOpenDefDoc because getCompilerModel is not supported
28092809
CompilerModel cm = getCompilerModel();
2810-
if (cm==null) {
2810+
if (cm == null) {
28112811
// use the cache adapter so setting the keywords doesn't load the document
28122812
_cacheAdapter.setKeywords(edu.rice.cs.drjava.model.compiler.JavacCompiler.JAVA_KEYWORDS);
28132813
}

‎drjava/src/edu/rice/cs/drjava/model/cache/DocumentCache.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ private DefinitionsDocument makeDocument() {
176176
_doc = _rec.make();
177177
assert _doc != null;
178178
// update documents if necessary
179-
if (_keywords != null) {
179+
if (_keywords != null) { // copy cached keywords to new copy of doc
180180
_doc.setKeywords(_keywords); _keywords.clear(); _keywords = null;
181181
}
182182
}

‎drjava/src/edu/rice/cs/drjava/model/definitions/DefinitionsDocument.java‎

Lines changed: 12 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -117,14 +117,15 @@ public void close() {
117117
/** Uses an updated version of the DefaultEditorKit */
118118
private final DefinitionsEditorKit _editor;
119119

120-
/* Unfortunately, we must use "this" as the lock object for Lock for _wrappedPosList because there is no lightweight
121-
* way to dynamically construct a dedicated lock for this variable. The lock is accessed by a super*-class during
122-
* object initialization (when no initialization has yet been performed for this class). In particular,
123-
* AbstractDocument calls createPosition in its <init> method. Java inexplicably forbids access to uninitialized
124-
* volatile variables. (Why? They have blank values just like blank final fields!) so a demand-driven
125-
* initialization scheme like double-check cannot be used. The locking expression could execute static code that uses
126-
* the value of "this" (passed as a parameter) to index a hashtable of locks that allocates new locks on demand
127-
* (simulating double check without the benefits of dynamic dispatch), but his is incredibly heavyweight. */
120+
/* Unfortunately, we must use the following shared static lock as the lock object for Lock for _wrappedPosList because
121+
* here is no lightweight way to dynamically construct a dedicated lock for this variable. The lock is accessed by a
122+
* super*-class (AbstractDocument calls createPostion) during object initialization (when no initialization has yet
123+
* been performed for this class). Java inexplicably forbids access to uninitialized volatile variables. (Why?
124+
* hey have blank values just like blank final fields!) so a demand-driven initialization scheme like double-check
125+
* cannot be used. Using "this" as a lock appear to create a deadlock. The "this" lock may be used for other
126+
* purposes. */
127+
128+
private static final Object _wrappedPosListLock = new Object();
128129

129130
/** List with weak references to positions. */
130131
private volatile LinkedList<WeakReference<WrappedPosition>> _wrappedPosList;
@@ -1188,7 +1189,7 @@ public Position createPosition(final int offset) throws BadLocationException {
11881189
// }
11891190
// }));
11901191
WrappedPosition wp = new WrappedPosition(createUnwrappedPosition(offset));
1191-
synchronized(this) {
1192+
synchronized(_wrappedPosListLock) {
11921193
if (_wrappedPosList == null) _wrappedPosList = new LinkedList<WeakReference<WrappedPosition>>();
11931194
_wrappedPosList.add(new WeakReference<WrappedPosition>(wp));
11941195
}
@@ -1201,7 +1202,7 @@ public Position createPosition(final int offset) throws BadLocationException {
12011202
*/
12021203
public WeakHashMap<WrappedPosition, Integer> getWrappedPositionOffsets() {
12031204
LinkedList<WeakReference<WrappedPosition>> newList = new LinkedList<WeakReference<WrappedPosition>>();
1204-
synchronized(this) {
1205+
synchronized(_wrappedPosListLock) {
12051206
if (_wrappedPosList == null) { _wrappedPosList = new LinkedList<WeakReference<WrappedPosition>>(); }
12061207
WeakHashMap<WrappedPosition, Integer> ret = new WeakHashMap<WrappedPosition, Integer>(_wrappedPosList.size());
12071208

@@ -1222,7 +1223,7 @@ public WeakHashMap<WrappedPosition, Integer> getWrappedPositionOffsets() {
12221223
* @param whm weakly-linked hashmap of wrapped positions and their offsets
12231224
*/
12241225
public void setWrappedPositionOffsets(WeakHashMap<WrappedPosition, Integer> whm) throws BadLocationException {
1225-
synchronized(this) {
1226+
synchronized(_wrappedPosListLock) {
12261227
if (_wrappedPosList == null) { _wrappedPosList = new LinkedList<WeakReference<WrappedPosition>>(); }
12271228
_wrappedPosList.clear();
12281229

‎drjava/src/edu/rice/cs/drjava/model/repl/newjvm/InterpreterJVM.java‎

Lines changed: 143 additions & 87 deletions
Large diffs are not rendered by default.

‎drjava/src/edu/rice/cs/drjava/ui/MainFrame.java‎

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3221,7 +3221,7 @@ public void run() {
32213221

32223222
// Timer to display a message if a debugging step takes a long time
32233223
_debugStepTimer = new Timer(DEBUG_STEP_TIMER_VALUE, new ActionListener() {
3224-
public void actionPerformed(ActionEvent e) { _model.printDebugMessage("Stepping..."); }
3224+
public void actionPerformed(ActionEvent e) { _model.printDebugMessage("Stepping ..."); }
32253225
});
32263226
_debugStepTimer.setRepeats(false);
32273227

@@ -5761,24 +5761,26 @@ void debuggerResume() throws DebugException {
57615761

57625762
/** Automatically traces through the entire program with a defined rate for stepping into each line of code*/
57635763
void debuggerAutomaticTrace() {
5764-
if(isDebuggerReady()) {
5764+
if (isDebuggerReady()) {
57655765
if(!_model.getDebugger().isAutomaticTraceEnabled()) {
57665766
try {
57675767
int rate = DrJava.getConfig().getSetting(OptionConstants.AUTO_STEP_RATE);
57685768

57695769
_automaticTraceTimer = new Timer(rate, new ActionListener() {
57705770
public void actionPerformed(ActionEvent e) {
5771+
_debugStepTimer.stop();
57715772
if (_model.getDebugger().isAutomaticTraceEnabled()) {
57725773
// hasn't been disabled in the meantime
57735774
debuggerStep(Debugger.StepType.STEP_INTO);
5774-
_debugStepTimer.restart(); // _debugStepTimer prints "Stepping..." when timer expires
5775+
// _debugStepTimer.restart(); // _debugStepTimer prints "Stepping..." when timer expires
57755776
}
57765777
}
57775778
});
57785779
_automaticTraceTimer.setRepeats(false);
57795780
_model.getDebugger().setAutomaticTraceEnabled(true);
57805781
_debugPanel.setAutomaticTraceButtonText();
57815782
debuggerStep(Debugger.StepType.STEP_INTO);
5783+
_debugStepTimer.stop();
57825784
}
57835785
catch (IllegalStateException ise) {
57845786
/* This may happen if the user if stepping very frequently, and is even more likely if they are using both
@@ -7985,7 +7987,7 @@ public void run() {
79857987
_threadPool.submit(new Runnable() {
79867988
public void run() {
79877989
Thread.currentThread().setPriority(UPDATER_PRIORITY);
7988-
synchronized (_updateLock) {
7990+
synchronized(_updateLock) {
79897991
try { // _pendingUpdate can be updated during waits
79907992
do {
79917993
_waitAgain = false;
@@ -8623,8 +8625,10 @@ public void run() {
86238625
public void stepRequested() {
86248626
// Print a message if step takes a long time; timer must be restarted on every step (automatic trace)
86258627
synchronized(_debugStepTimer) {
8626-
if (! _debugStepTimer.isRunning()) _debugStepTimer.start();
8627-
else _debugStepTimer.restart();
8628+
if (! _automaticTraceTimer.isRunning()) {
8629+
if (! _debugStepTimer.isRunning()) _debugStepTimer.start();
8630+
else _debugStepTimer.restart();
8631+
}
86288632
}
86298633
}
86308634

@@ -8636,8 +8640,7 @@ public void currThreadSuspended() {
86368640
if(_model.getDebugger().isAutomaticTraceEnabled()) {
86378641
//System.out.println("new _automaticTraceTimer AUTO_STEP_RATE=" + AUTO_STEP_RATE + ", " +
86388642
// System.identityHashCode(_automaticTraceTimer);
8639-
if ((_automaticTraceTimer != null) && (! _automaticTraceTimer.isRunning()))
8640-
_automaticTraceTimer.start();
8643+
if ((_automaticTraceTimer != null) && (! _automaticTraceTimer.isRunning())) _automaticTraceTimer.start();
86418644
}
86428645
}
86438646

@@ -8660,9 +8663,11 @@ public void threadLocationUpdated(OpenDefinitionsDocument doc, int lineNumber, b
86608663
/* Must be executed in event thread. */
86618664
public void currThreadDied() {
86628665
assert EventQueue.isDispatchThread();
8663-
_disableStepTimer();
86648666
_model.getDebugger().setAutomaticTraceEnabled(false);
8665-
if (_automaticTraceTimer != null) _automaticTraceTimer.stop();
8667+
if (_automaticTraceTimer != null) {
8668+
_automaticTraceTimer.stop();
8669+
}
8670+
_disableStepTimer();
86668671
if (isDebuggerReady()) {
86678672
try {
86688673
if (!_model.getDebugger().hasSuspendedThreads()) {

‎drjava/src/edu/rice/cs/util/JoinInputStream.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ public synchronized void run() {
212212
exception = ex;
213213
len = -1;
214214
}
215-
synchronized (monitor) {
215+
synchronized(monitor) {
216216
available = len;
217217
pos = 0;
218218
monitor.notify();

‎drjava/src/edu/rice/cs/util/docnavigation/AWTContainerNavigatorFactory.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,7 @@ public IDocumentNavigator<ItemT> makeListNavigator(final IDocumentNavigator<Item
6767
final IDocumentNavigator<ItemT> child = makeListNavigator();
6868
Utilities.invokeLater(new Runnable() {
6969
public void run() {
70-
// synchronized (child.getModelLock()) { // dropped because of cost; each atomic action is still synchronized
70+
// synchronized(child.getModelLock()) { // dropped because of cost; each atomic action is still synchronized
7171
migrateNavigatorItems(child, parent);
7272
migrateListeners(child, parent);
7373
}
@@ -88,7 +88,7 @@ public IDocumentNavigator<ItemT> makeTreeNavigator(String name, final IDocumentN
8888
final IDocumentNavigator<ItemT> child = makeTreeNavigator(name);
8989
Utilities.invokeLater(new Runnable() {
9090
public void run() {
91-
// synchronized (child.getModelLock()) { // dropped because of cost; each atomic action is still synchronized
91+
// synchronized(child.getModelLock()) { // dropped because of cost; each atomic action is still synchronized
9292
for(Pair<String, INavigatorItemFilter<ItemT>> p: l) { child.addTopLevelGroup(p.first(), p.second()); }
9393
migrateNavigatorItems(child, parent);
9494
migrateListeners(child, parent);

‎drjava/src/edu/rice/cs/util/docnavigation/JTreeSortNavigator.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -387,7 +387,7 @@ public void refreshDocument(ItemT doc, String path) {
387387
/** Sets the specified document to be active (current). Only executes in the event thread. */
388388
public void selectDocument(ItemT doc) {
389389
assert EventQueue.isDispatchThread();
390-
// synchronized (_model) { // lock out mutation
390+
// synchronized(_model) { // lock out mutation
391391
DefaultMutableTreeNode node = _doc2node.get(doc);
392392
if (node == null) return; // doc is not in the navigator
393393
if (node == _current) return; // current doc is the active doc
@@ -907,7 +907,7 @@ public String[] getCollapsedPaths() {
907907
assert (EventQueue.isDispatchThread() || Utilities.TEST_MODE);
908908

909909
ArrayList<String> list = new ArrayList<String>();
910-
// synchronized (_model) {
910+
// synchronized(_model) {
911911
DefaultMutableTreeNode rootNode = (DefaultMutableTreeNode)_model.getRoot();
912912
// We use a raw type here because depthFirstEnumeration() has a raw type signature
913913
Enumeration<?> nodes = rootNode.depthFirstEnumeration(); /** This warning is expected **/
@@ -931,7 +931,7 @@ public String[] getCollapsedPaths() {
931931
*/
932932
public String generatePathString(TreePath tp) {
933933
String path = "";
934-
// synchronized (_model) {
934+
// synchronized(_model) {
935935
TreeNode root = (TreeNode) _model.getRoot();
936936

937937
while (tp != null) {
@@ -947,7 +947,7 @@ public String generatePathString(TreePath tp) {
947947

948948
/** If the currently selected item is not an INavigatorItem, select the one given. Only runs in event thread. */
949949
public void requestSelectionUpdate(ItemT ini) {
950-
// synchronized (_model) {
950+
// synchronized(_model) {
951951
// This code checked if the selected node was a leaf, i.e. a Java source file,
952952
// and if it wasn't, reverts the selection. The code was commented out to allow
953953
// selection of file nodes (i.e. folders) and string nodes (i.e. [ Source Files ], etc.)

‎drjava/src/edu/rice/cs/util/newjvm/AbstractSlaveJVM.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ public final synchronized void quit() {
7373
new Thread(_quitSlaveThreadName) {
7474
public void run() {
7575
// ensure (as best we can) that the quit() RMI call has returned cleanly
76-
synchronized (AbstractSlaveJVM.this) {
76+
synchronized(AbstractSlaveJVM.this) {
7777
try { System.exit(0); }
7878
catch (RuntimeException e) { error.log("Can't invoke System.exit", e); }
7979
}

0 commit comments

Comments
 (0)