Dynamo UI should not block during concurrent save/run operations- attempt 1 #12564

mjkkirschner · 2022-01-20T19:32:37Z

Purpose

while investigating https://jira.autodesk.com/browse/DYN-4521 I found that the new ExternalFiles implementation causes a UI hang when when called while the graph is executing. (more specifically when the LiveRunner has a lock on its mutexObject field) This is commonly during graph executions.

This is attempt 1 using an existing simple bool field (HomeWorkspaceModel.executingTask) to track if a graph run is in progress. I'm not sure how robust a solution it is, and it's not covering all cases because a client could call some other EngineController or LiveRunner method that grabs this lock - though it will likely work for most cases, is simple, and works in my repro case.

This means that save will still occur during graph execution as before but ExternalFile ref calculation will be skipped if the graph is executing.

~~Also note I have not yet verified this is the same issue as in Alias.~~ I have verified this fixes the hang in Alias. (well at least the one I experienced during testing).

I've also removed the HeartBeat thread as it appears it was only used for instrumentation and logging uptime to the registry which doesn't appear to be used anywhere.

To test this is quite simple, run a long running graph and hit the save icon.

Declarations

Check these if you believe they are true

The codebase is in a better state after this PR
Is documented according to the standards
The level of testing this PR includes is appropriate
User facing strings, if any, are extracted into *.resx files
All tests pass using the self-service CI.
Snapshot of UI changes, if any.
Changes to the API follow Semantic Versioning and are documented in the API Changes document.
This PR modifies some build requirements and the readme is updated

Release Notes

Fixes a UI hang when saving at the same time the graph is running.

mjkkirschner · 2022-01-21T00:22:02Z

⚠️
This doesn't work in some situations - for example see the failing test - It looks like executingTask is set to true AFTER it's set to false, so the state is actually inverted. I think this is because the tests put the scheduler into synchronous mode (like Revit) - I don't think this solution will work well in those contexts.

Maybe it could be adapted with another bool, for example RunEnabled seems to be reset at the correct time... not sure how it will work with different thread scenarios though... leaning towards solution 2 for now.

mjkkirschner · 2022-01-21T00:35:10Z

RunEnabled seems to work, waiting for tests.

pinzart90 · 2022-01-21T16:41:33Z

I would rather go with this option.
The second one could probably be made a bit more generic and we could try to look at other similar situations

pinzart90

LGTM

mjkkirschner · 2022-01-21T18:11:46Z

this passed on the serial CI as well.

mjkkirschner · 2022-01-21T18:14:29Z

spoke with @pinzart90 about this, given the short timeline we agreed that while this PR does not cover all cases, it's sufficient and it's better to have less changes to test.

@aparajit-pratap @sm6srw please prioritize this PR over attempt 2.

I am working to verify this fixes the reported alias hang.

pinzart90

LGTM

…empt 1 (DynamoDS#12564) * attempt 1, this works but seems odd. * switch executingTask for RunEnabled * put external files back in wsm * int to private * remove usings * revert change * revert change Co-authored-by: michael kirschner <[email protected]>

* Dynamo UI should not block during concurrent save/run operations- attempt 1 (#12564) * attempt 1, this works but seems odd. * switch executingTask for RunEnabled * put external files back in wsm * int to private * remove usings * revert change * revert change Co-authored-by: michael kirschner <[email protected]> * remove heartbeat * remove heartbeat.cs Co-authored-by: michael kirschner <[email protected]>

aparajit-pratap · 2022-01-23T22:44:28Z

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

+            // If an execution is in progress we'll have to wait for it to be done before we can gather the
+            // external file references as this implementation relies on the output values of each node.
+            //instead just bail to avoid blocking the UI.
+            if (this is HomeWorkspaceModel homeWorkspaceModel && homeWorkspaceModel.RunSettings.RunEnabled)


If retrieving external files is skipped here due to a graph running, say when saving a graph, then what action will cause this computation to actually run again at a later time?

In other words, how do we ensure that this is scheduled to run after the graph finishes executing?

currently as merged - the user will need to save the graph again, backup save etc while the graph is not running for this computation to run.

We could likely schedule another save if this condition is encountered by using the scheduler. I have some hesitation about this though - if the user tried to manually save their graph as execution starts, and then we schedule another save for directly after - if execution somehow modifies the graph then I guess this could be considered destructive, the example I'm thinking of is element binding, but there could be others... will think about it.

It's also important to note that this issue is only present in sandbox builds (where UI and Scheduler thread are different).

attempt 1, this works but seems odd.

f8054e2

mjkkirschner requested review from pinzart90, sm6srw and aparajit-pratap January 20, 2022 19:34

switch executingTask for RunEnabled

61cec5b

pinzart90 approved these changes Jan 21, 2022

View reviewed changes

put external files back in wsm

25d378a

mjkkirschner added 4 commits January 21, 2022 13:14

int to private

5c13fda

remove usings

081354f

revert change

dba9b84

revert change

7bc8f84

pinzart90 approved these changes Jan 21, 2022

View reviewed changes

mjkkirschner merged commit f784c5e into DynamoDS:master Jan 21, 2022

mjkkirschner mentioned this pull request Jan 21, 2022

2.13.1 fix ui hang when concurrent run and save. #12570

Merged

aparajit-pratap reviewed Jan 23, 2022

View reviewed changes

mjkkirschner mentioned this pull request Jan 25, 2022

Add test for GetExternalFiles not executing concurrently with graph execution #12583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamo UI should not block during concurrent save/run operations- attempt 1 #12564

Dynamo UI should not block during concurrent save/run operations- attempt 1 #12564

mjkkirschner commented Jan 20, 2022 •

edited

Loading

mjkkirschner commented Jan 21, 2022

mjkkirschner commented Jan 21, 2022

pinzart90 commented Jan 21, 2022

pinzart90 left a comment

mjkkirschner commented Jan 21, 2022

mjkkirschner commented Jan 21, 2022

pinzart90 left a comment

aparajit-pratap Jan 23, 2022

aparajit-pratap Jan 23, 2022

mjkkirschner Jan 24, 2022

Dynamo UI should not block during concurrent save/run operations- attempt 1 #12564

Dynamo UI should not block during concurrent save/run operations- attempt 1 #12564

Conversation

mjkkirschner commented Jan 20, 2022 • edited Loading

Purpose

Declarations

Release Notes

mjkkirschner commented Jan 21, 2022

mjkkirschner commented Jan 21, 2022

pinzart90 commented Jan 21, 2022

pinzart90 left a comment

Choose a reason for hiding this comment

mjkkirschner commented Jan 21, 2022

mjkkirschner commented Jan 21, 2022

pinzart90 left a comment

Choose a reason for hiding this comment

aparajit-pratap Jan 23, 2022

Choose a reason for hiding this comment

aparajit-pratap Jan 23, 2022

Choose a reason for hiding this comment

mjkkirschner Jan 24, 2022

Choose a reason for hiding this comment

mjkkirschner commented Jan 20, 2022 •

edited

Loading