edwardkort / WWIDesigner

Wood Wind Instrument Designer
43 stars 8 forks source link

Exception in a StudyView Activity can hang application #51

Closed bhp1 closed 7 years ago

bhp1 commented 8 years ago

At times an exception thrown in a StudyView Activity, or some other premature return without presenting a new JFrame, can cause the application to hang; the BlockingProgressListener is presented, but cannot be closed. Application must be killed from operating system.

For example: open an instrument and tuning, select the tuning tab, then select Window->Toggle data view then Tool->Graph note tuning. This should provoke a null pointer exception because no fingering is available; usually StudyView handles the exception, but sometimes this results in the hang described above.

edwardkort commented 8 years ago

I see this with Tool->Graph spectrum, but not with Tool->Graph tuning. There are two issues here (at least):

  1. We never wrote any code to cancel a BlockingProgressListener.
  2. Why doesn't Graph spectrum work the same way (display the spectrum for the lowest note, if a fingering is not selected) for the tabular and XML views?
bhp1 commented 8 years ago

At present, graph spectrum needs a tuning panel to retrieve the note from; it can't retrieve one Fingering object from an XML view.

You aren't likely to see this problem with the other tools, because I don't know how to provoke an exception, or an early exit, from any of them.

By rights, Graph spectrum should be greyed out when the XML view is shown, because getSelectedFingering is returning null, but for that to happen, WIDesigner would have to call StudyView::updateActions(), which is currently protected, when it calls XMLToggleView::toggleView().

edwardkort commented 8 years ago

There must be a flow that, in an activity, catches an exception before invoking the registered progress listener. However, there are other issues in play.

Tool items should use the selections in the Study pane. However, graphNote ignores the Tuning selection in that pane. This allows a hole-count mismatch between the instrument selected in the Study pane and the Tuning displayed in the Editor pane. If you have this mismatch, the program also hangs in the progress listener - even though a Tuning panel (not XML) is displayed.

It appears that the wiring for Graph note needs some redesign, not just a simple Exception handler. JMTC.

bhp1 commented 8 years ago

You have a point about the design of Graph spectrum, but that's not what I wanted to capture in this issue. When and if there are problems in the tools, the progress listener framework doesn't always cope properly. It isn't reliably repeatable, and at the moment I don't know where the fault is or how to fix it. It could, in principle, happen with any tool, and the consequences when it does are drastic.

edwardkort commented 8 years ago

I'll try to understand the exception flow in Graph note. Exception handling seems to work correctly in the other activities (at least the hole-mismatch exceptions), unless you see errors elsewhere.

I spent a day trying to implement a cancel in the progress listener, with no luck. Tried all the patterns in the documentation. Do you have any ideas?

edwardkort commented 8 years ago

I don't know what is different about the graphNote action that requires a thrown exceptions from its activity unlike the other actions. But the fix, which explicitly rethrows the exception from studyview.graphNote seems to do the trick (although it does pollute the console with the stack trace).

You might consider doing some work on the updateActions integration for the graphNote action: maybe fire it after clicking the change-view button. For your original scenario, after getting the exception with the XML tuning view, you must change the view back, click on another tab in the editor pane, and then click back on the tuning tab in order to activate the graph note action.

bhp1 commented 8 years ago

I've found that the hang results from an interaction between MessageDialogRequest.showMessageDialog and SwingUtilities.invokeLater or the JDialog in BlockingProgressListener. If one of the tools calls showMessageDialog before, or instead of, calling study.calculateTuning, or whatever, most of the time the application will display both the message dialog and the intended tuning table or graph at the same time. However, every now and then, the application will hang, showing only the progress listener. This is also true for the showMessageDialog in showException, so throwing an exception from any of these tools can cause the hang. The workaround code you added to StudyView is thus still susceptible.

edwardkort commented 8 years ago

I am no longer able, with the fix I put in, to generate any hangs - under any scenario I have tried. Do you have a testable scenario? And what version of java, jide, and jdaf are you using?

bhp1 commented 8 years ago

Yesterday, I tried once, and it hung. Today, I tried a dozen times and it didn't. To make it easier to replicate, temporarily replace one of the tool calls such as study.calculateTuning with:

    MessageDialogRequest.showMessageDialog(getApplication(),
        "Click OK to continue", "Ready to continue", MessageDialogRequest.INFORMATION_STYLE);

You may have to try many times, but eventually you're likely to get a hang. My current suspicion is that it is a race condition: showMessageDialog and BlockingProcessListener are both trying to display their dialogs with the same thread, and that isn't the same thread as the tools run under. Deadlock results if BlockingProcessListener gets there first.

JRE: 1.8.0 build 91 JIDE: 3.6.12 JDAF: 2.4.26

edwardkort commented 8 years ago

I am using jide 3.6.14. The latest release is 3.6.15. There have been numerous bug fixes since 3.6.12 that may be relevant. I suggest updating the libraries and revisiting this issue.

bhp1 commented 8 years ago

More details on the deadlock: BlockingProgressListener.progressStart is executing on the AWT thread. The thread owns Component$AWTTreeLock, and is waiting on ButtonPanelLayout. Meanwhile, MessageDialogRequest.showMessageDialog is executing in a timer thread; that thread owns ButtonPanelLayout and is waiting on Component$AWTTreeLock. MessageDialogRequest.showMessageDialog, being a GUI operation, should be executing on the AWT thread to be safe.

bhp1 commented 8 years ago

I've got a fix that uses SwingUtilities.invokeLater to put the MessageDialogRequest on the AWT thread, where it belongs. While I've got the hood open, I think I know how to add a Cancel button for long optimizations.

bhp1 commented 8 years ago

While I can add a Cancel button, there's more work required before our tools support it. This issue, at least, is now fixed. In future, we'll need to be more careful that our tool activities don't throw up message dialogs or other GUI elements not on the AWT thread.

edwardkort commented 7 years ago

Closed with release v2.0.0.