sandbox should not crash silently #10014

mjkkirschner · 2019-09-27T05:33:04Z

Purpose

Dynamo Sandbox should not crash silently.

To avoid this snippet of stack trace and exception are shown in a message box from system.windows (not forms) to avoid adding any other dependencies that might fail to load.
ASM load is now wrapped in a try catch
okay button in message box navigates to dynamo builds wiki page with info on using sandbox.
filter out ASM locations that don't include tsplinesA - as these installs of ASM will not work with Dynamo. Discovered this as a cause of a silent crash with help from users on the forum and internal users.

UPDATE:

changed filemask to ASMH*A.dll to better match what dependencies we actually have, and also filter results that don't include tbb and tbbmalloc.dll
raise an event when preload fails and trigger a messagebox to note the location we tried to load from - will be useful if we encounter other odd geometry library configurations in the future.
Dynamo will now start, even if the preload fails. Geometry nodes will of course not work though.

TODO:

use resx files

Declarations

Check these if you believe they are true

The code base 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.

Reviewers

@aparajit-pratap
@ColinDayOrg

FYIs

@TheBIMsider

filter out ASM locations which dont include tsplines A

aparajit-pratap · 2019-09-27T14:39:36Z

src/Tools/DynamoShapeManager/Utilities.cs

+            }
+            catch
+            {
+                Console.WriteLine("Failed to load ASM binaries");


Won't the exception be silently swallowed out here? I don't see a throw?

yes, okay, after discussing a bit I see what you're saying - I think instead of Dynamo starting with no geometry - it is likely more useful to throw and message to the user where the geometry library failed to load from - this will also help us later get to the bottom of crash reports without needing to recreate the users local adsk install setup.

I guess I'm okay with launching Dynamo without the geometry library but in addition to doing that I think it would be useful to display a message to users saying that the geometry library failed to load due to these reasons but if they wish they can still continue to use Dynamo.

I think at this point doing either or is the least risky thing, we don't have access to the dynamo logger here - and if we have to choose right now then I think showing the offending path to the user is actually more useful.

I guess I meant to ask if it's possible to show the message box, the user clicks "ok", and is able to continue to use Dynamo?

I don't think so - not without a larger refactoring - but I can look at that if you think it's worth it.

hmm - for some context, we should not introduce a dependency on system.windows into the preloader - which needs to be cross platform - but the other message box raised in dynamoSandbox only is shown when it is too late to recover.

@aparajit-pratap PTAL - figured out a way to do this while avoiding big set of changes or new public apis. We may want to do that as part of the followup we discussed.

throw exception with more info when asm fails to load fix bug in message from old method add new line to message dialog

QilongTang · 2019-09-27T16:33:09Z

src/DynamoSandbox/DynamoCoreSetup.cs

+                    {
+                        //show a message dialog box with the exception so the user
+                        //can effectively report the issue.
+                        var shortStackTrace = String.Join(Environment.NewLine,e.StackTrace.Split(Environment.NewLine.ToCharArray()).Take(10));


Since this is pointing to the wiki, we will need to update there how to file issue related to start up failure.

yep, I will do that there.

QilongTang · 2019-09-27T16:42:00Z

src/Tools/DynamoShapeManager/Utilities.cs

+            }
+            catch
+            {
+                throw new FileNotFoundException($"Could not load geometry library binaries from : {asmLocation}");


Is this the only kind of cause exception? @mjkkirschner @aparajit-pratap

this should just be Exception - I will change it.

aparajit-pratap · 2019-09-27T17:27:24Z

@mjkkirschner I'm a little confused. With your first commit (without rethrowing from the try-catch around ASM preloading), how were you able to display the message box with the stacktrace around ASM preload? Wouldn't it have simply swallowed that exception and launched Dynamo?

aparajit-pratap · 2019-09-27T17:29:08Z

src/Tools/DynamoShapeManager/Utilities.cs


+                //filter install locations missing tsplinesA
+                installations = installations.Cast<KeyValuePair<string, Tuple<int,int,int,int>>>().
+                    Where(x => Directory.EnumerateFiles(x.Key, "tsplines*A.dll").Any());


Does it make sense to fold this check into getASMInstallsFunc? Also my previous question was why don't we do this check for "ASM*A.dll" as well?

I can try to fold it in there -

we're already checking for that in the findProductInstallations function when we pass "ASMAHL*.dll" isn't this - essentially doing the same check twice?

There could be an ASMAHL*A.dll and an ASMAHL*I.dll?

kind of, the offending folder has the ASMA.dlls and tsplinesI.dll

mjkkirschner · 2019-09-27T17:32:21Z

@mjkkirschner I'm a little confused. With your first commit (without rethrowing from the try-catch around ASM preloading), how were you able to display the message box with the stacktrace around ASM preload? Wouldn't it have simply swallowed that exception and launched Dynamo?

the first commit would have swallowed the exception and started dynamo - the message box I showed in the image was for any other unhandled exception - for example - if all the dlls are blocked.

aparajit-pratap · 2019-09-27T17:40:26Z

the first commit would have swallowed the exception and started dynamo - the message box I showed in the image was for any other unhandled exception - for example - if all the dlls are blocked.

So the message box showing ASM related exception was just a contrived example?

mjkkirschner · 2019-09-27T17:40:59Z

the first commit would have swallowed the exception and started dynamo - the message box I showed in the image was for any other unhandled exception - for example - if all the dlls are blocked.

So the message box showing ASM related exception was just a contrived example?

yes - I threw away all the libG folders 😉

…to only search for asm version we use

use $string interpolation for some old messages log to console for CLI clients

aparajit-pratap · 2019-09-30T00:08:07Z

src/Tools/DynamoShapeManager/Utilities.cs

+
+            //filter install locations missing tbb and tbbmalloc.dll
+            return installs.Cast<KeyValuePair<string, Tuple<int, int, int, int>>>().
+                Where(x => Directory.EnumerateFiles(x.Key, "tbb*.dll").Count() >1);


I think we can make this check more strict by specifically checking for both tbb.dll and tbbmalloc.dll just so we don't have any other surprises like tbbX.dll being present but not being the same as one or the other above.

okay, will do.

aparajit-pratap · 2019-09-30T15:35:17Z

Thanks, LGTM!

mjkkirschner · 2019-09-30T20:17:46Z

@smangarole brining this in, will cherry pick next.

add message box when crash occurs in sandbox before view is loaded

44bfb8f

filter out ASM locations which dont include tsplines A

aparajit-pratap reviewed Sep 27, 2019

View reviewed changes

mjkkirschner changed the title ~~[WIP] sandbox should not crash silently~~ sandbox should not crash silently Sep 27, 2019

mjkkirschner added the PTAL Please Take A Look 👀 label Sep 27, 2019

mjkkirschner added 2 commits September 27, 2019 11:55

use resx files

27ad0c2

throw exception with more info when asm fails to load fix bug in message from old method add new line to message dialog

update message and formatting

c67b28e

QilongTang reviewed Sep 27, 2019

View reviewed changes

aparajit-pratap reviewed Sep 27, 2019

View reviewed changes

mjkkirschner added 3 commits September 27, 2019 15:45

filter for tbb and tbbmalloc instead of tsplines and change filemask …

9b88fbc

…to only search for asm version we use

static event used to signal asm preload failure and raise message box

7335215

use $string interpolation for some old messages log to console for CLI clients

fix test failure

b1d5fc9

aparajit-pratap reviewed Sep 30, 2019

View reviewed changes

review comments

7d4f5fe

aparajit-pratap added LGTM Looks good to me and removed PTAL Please Take A Look 👀 labels Sep 30, 2019

mjkkirschner merged commit 2c4e82b into DynamoDS:master Sep 30, 2019

mjkkirschner mentioned this pull request Sep 30, 2019

2.4.1 - Sandbox should not crash silently #10021

Merged

horatiubota added the error/warning/crash Issues mentioning a Dynamo error, warning or crash message label Nov 11, 2019

sandbox should not crash silently #10014

sandbox should not crash silently #10014

Uh oh!

Conversation

mjkkirschner commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

UPDATE:

TODO:

Declarations

Reviewers

FYIs

Uh oh!

aparajit-pratap Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aparajit-pratap Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Sep 27, 2019

Uh oh!

aparajit-pratap commented Sep 27, 2019

Uh oh!

mjkkirschner commented Sep 27, 2019

Uh oh!

aparajit-pratap Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap commented Sep 30, 2019

Uh oh!

mjkkirschner commented Sep 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mjkkirschner commented Sep 27, 2019 •

edited

Loading

aparajit-pratap Sep 27, 2019 •

edited

Loading

aparajit-pratap Sep 27, 2019 •

edited

Loading

aparajit-pratap commented Sep 27, 2019 •

edited

Loading

aparajit-pratap Sep 27, 2019 •

edited

Loading

aparajit-pratap Sep 27, 2019 •

edited

Loading

aparajit-pratap Sep 30, 2019 •

edited

Loading