Make scheduled/deferred task order deterministic

Fixes #920 #1143

Explanation from @robnee on Feb 7, 2021 in pull request #927

Solvespace uses two timers (generateAllTimer and showTWTimer) to defer tasks
until the event loop processing finishes. This helps coalesce multiple calls
into one. You can call scheduleGenerateAll multiple times while processing UI
messages but only trigger one GenerateAll. scheduleGenerateAll and
scheduleShowTW do their scheuduling by setting timers with durations of zero.
These timers fire (at least on Linux and Windows) some time after all other
events in the message queue have been processed. This works fine when
scheduling either one of these tasks. However, there is no guarantee in what
order the timers will fire (at least on Windows) regardless of which order the
scheduling calls are made. It's pretty easy to demonstrate (on some platforms)
by adding logging to the scheduling calls and timer callbacks.

In many cases TextWindow::Show depends on generateAll happening first. This
causes UI glitches where displays don't update and their contents are stale.
Since this behavior is not deterministic it's easy to imagine how this problem
could make certain bug reports difficult to reproduce and diagnose. #920 is a
good example. It also makes syncing up UI behavior across all platforms a
challenge.

Solving this in the platform domain is tricky. This is PR endeavors to make the
ordering of deferred calls to TextWindow::Show and generateAll deterministic.
It does this by replacing generateAllTimer and showTWTimer with a single
refreshTimer. Calls to scheduleGenerateAll and scheduleShowTW set flags to note
the requested operations and schedule the refreshTimer. A new callback function
SolveSpaceUI::Refresh can then check the flags and ensure that generateAll
happens first. It fixes #920. Moreover, this PR makes it easy to observe and
reproduce this problem reliably and across all platforms by simply reordering
the calls in the Refresh callback.

It's pretty clear that the ordering is important so some solution is needed, if
for no other reason than the sanity of the devs. I think this is a pretty good
solution as it spells out the ordering. If nothing else this PR is helpful in
further investigations.

@ruevs @phkahler I'd like to hear your thoughts.
pull/1259/head
robnee 2021-02-07 10:10:41 -05:00 committed by Paul Kahler
parent be3489e3a0
commit b48ce240c7
2 changed files with 22 additions and 10 deletions

View File

@ -125,12 +125,8 @@ void SolveSpaceUI::Init() {
SetLocale(locale);
}
generateAllTimer = Platform::CreateTimer();
generateAllTimer->onTimeout = std::bind(&SolveSpaceUI::GenerateAll, &SS, Generate::DIRTY,
/*andFindFree=*/false, /*genForBBox=*/false);
showTWTimer = Platform::CreateTimer();
showTWTimer->onTimeout = std::bind(&TextWindow::Show, &TW);
refreshTimer = Platform::CreateTimer();
refreshTimer->onTimeout = std::bind(&SolveSpaceUI::Refresh, &SS);
autosaveTimer = Platform::CreateTimer();
autosaveTimer->onTimeout = std::bind(&SolveSpaceUI::Autosave, &SS);
@ -302,12 +298,26 @@ void SolveSpaceUI::Exit() {
Platform::ExitGui();
}
void SolveSpaceUI::Refresh() {
// generateAll must happen bfore updating displays
if(scheduledGenerateAll) {
GenerateAll(Generate::DIRTY, /*andFindFree=*/false, /*genForBBox=*/false);
scheduledGenerateAll = false;
}
if(scheduledShowTW) {
TW.Show();
scheduledShowTW = false;
}
}
void SolveSpaceUI::ScheduleGenerateAll() {
generateAllTimer->RunAfterProcessingEvents();
scheduledGenerateAll = true;
refreshTimer->RunAfterProcessingEvents();
}
void SolveSpaceUI::ScheduleShowTW() {
showTWTimer->RunAfterProcessingEvents();
scheduledShowTW = true;
refreshTimer->RunAfterProcessingEvents();
}
void SolveSpaceUI::ScheduleAutosave() {

View File

@ -793,9 +793,11 @@ public:
// the sketch!
bool allConsistent;
Platform::TimerRef showTWTimer;
Platform::TimerRef generateAllTimer;
bool scheduledGenerateAll;
bool scheduledShowTW;
Platform::TimerRef refreshTimer;
Platform::TimerRef autosaveTimer;
void Refresh();
void ScheduleShowTW();
void ScheduleGenerateAll();
void ScheduleAutosave();