Exceptions thrown can sometimes result in two error dialogs

[PathogenDavid]

This came up as an unrelated bug discovered via #1787

This happens due to the two HandleWorkflowError calls below:

bonsai/Bonsai.Editor/EditorForm.cs

Lines 1229 to 1241 in 88a2412

	catch (Exception ex)
	{
	HandleWorkflowError(ex);
	shutdown.Dispose();
	throw;
	}
	},
	resource => resource.Workflow.TakeUntil(workflowBuilder.Workflow
	.InspectErrorsEx()
	.Do(RegisterWorkflowError)
	.IgnoreElements()))
	.SubscribeOn(NewThreadScheduler.Default.Catch<Exception>(HandleSchedulerError))
	.Subscribe(unit => { }, HandleWorkflowError, HandleWorkflowCompleted);

First the call on line 1231 invokes the main thread to display the error, then the exception is rethrown on line 1233. It's then caught a second time by the onError handler on line 1241.

If I remember correctly, during a more typical exception the first call will show a dialog but the second one will only highlight the node and put the exception in the status bar.

This difference comes down to the status of building in the first branch here:

bonsai/Bonsai.Editor/EditorForm.cs

Lines 1413 to 1418 in 88a2412

	if (workflowException != null && workflowException.Builder != null || exceptionCache.TryGetValue(e, out workflowException))
	{
	workflowError = workflowException;
	HighlightExceptionBuilderNode(workflowException, building);
	}
	else editorSite.ShowError(e.Message, Name);

It is however worth noting that two dialogs will appear no matter what when the second branch is entered. (As with the original 1787 fix.) So even if this comes down to something related to building there's still the potential for issues.

The handle call on line 1231 was added by 724b2e0 which was trying to fix a bug introduced by 9c8e390 for #908

We theorized that potentially this fix was actually trying to fix what was actually a race condition in reading building by HandleWorkflowError, but there's not enough context to say for sure. (See #1795)

We agreed it didn't make much sense to have two HandleWorkflowError calls here, but because the issue supposedly fixed by 9c8e390 is pretty serious (two error dialogs is practically benign compared to completely silent errors), we don't want to risk a regression here by reverting that commit blindly and decided this issue warrants further investigation.

[glopesdev]

@PathogenDavid interestingly, on my build and machine I only ever see one dialog, even though both code paths do get triggered.

Even more interesting, if I comment the first call site, I don't see a popup, which was the situation presumably fixed by 724b2e0.

Digging deeper, it looks like we are seeing the effects of capturing the building state-variable, since the second call starts with building set to true but by the time the variable is actually checked inside the BeginInvoke call, it is already set to false.

[glopesdev]

I have identified one possible candidate for the race condition behavior, which is the use of ScheduledDisposable in the ShutdownSequence method:

bonsai/Bonsai.Editor/EditorForm.cs

Line 1172 in 88a2412

return new ScheduledDisposable(new ControlScheduler(this), Disposable.Create(() =>

Effectively this means that the call to Dispose runs inside a BeginInvoke to the main dialog. Because both HandleWorkflowError and ShutdownSequence were queueing invokes, there is a race in the message queue. In my case the race seems to be deterministic, and meant that by calling HandleWorkflowError before ShutdownSequence I was able to trust that the first call to show the dialog would always come before the shutdown.

I'm not sure how in your case you were able to get the second call scheduled before the dispose, but in any case I think using the fix proposed in #1795 together with removal of the first call to HandleWorkflowError should resolve this completely.

[PathogenDavid]

interestingly, on my build and machine I only ever see one dialog, even though both code paths do get triggered.

This is because of the move from InvalidOperationException to WorkflowBuildException, as that alone fixed this issue indirectly.

---

race condition stuff

So looking at it again, the capture in #1795 is actually not really the right thing to do here. It actually creates a race condition rather than preventing it.

When we were in our call I overlooked the fact that ShutdownSequence was being deferred to the UI thread as well. This actually means that capturing is causing a race condition between the UI thread setting building to false and the background thread capturing building in HandleWorkflowError.

As such capturing building is not the right thing to do here.

there is a race in the message queue. In my case the race seems to be deterministic

I checked the implementation of BeginInvoke here. Multiple BeginInvoke invocations scheduled by the same background thread will always be completed in the order they were submitted.

There's no race concern here, but is there a reason we're using BeginInvoke so aggressively? IMO fire and forget should be the exception, not the rule.

Hiding things in the ScheduledDisposable that is only sometimes disposed at certain places also isn't ideal IMO. It seems like ShutdownSequence should just be a method called from a catch or finally clause as needed based on the context.

---

As for this issue (the two error dialogs), I think the real problem here is caused these two facts:

1. HandleWorkflowError is called twice (and we don't know why.)
2. HandleWorkflowError will always show a dialog for non-workflow exceptions.

These two facts combine mean that non-workflow exceptions (as with the original implementation of #1787) will always show two dialogs.

It might be easier to just handle the second fact for now and worry about the first one for Bonsai 3.0. I think EditorForm likely gets refactored to accomplish some of the 3.0 goals and refactoring probably properly fixes things anyway.

[PathogenDavid]

Found another case which appears to trigger this: bonsai-rx/gui#29