[Cherry-pick] DYN-2101 Installing ampersand package crashes Dynamo while the graph is open #9951
Conversation
| foreach (var pkg in enumerable) | ||
| { | ||
| TryLoadPackageIntoLibrary(pkg); | ||
| } | ||
|
|
||
| // Setting back the DisableRun property back to false, as the package loading is completed. | ||
| EngineController.DisableRun = false; |
There was a problem hiding this comment.
is there ever a case where we could accidentally turn runs off and then never turn them back on?
There was a problem hiding this comment.
I don't think so as this would be set back to false, once the package loading is completed. I did not run into any such case.
There was a problem hiding this comment.
I think @mjkkirschner was alluding to a case where an exception is thrown from TryLoadPackageIntoLibrary and this flag-reset step is skipped in that case. I think TryLoadPackageIntoLibrary has a try-catch inside it that can catch any exception and not rethrow it in order for this flag reset to take place right after but to be safe we can have another try-catch-finally around the for loop and reset the flag in the finally block to ensure it always happens no matter what.
There was a problem hiding this comment.
@aparajit-pratap thats exactly what I was thinking - do you think it's worth doing this:
or should it be okay since TryLoadPackageIntoLibrary will already catch and move onto the next package?
| EngineController.DisableRun = false; | |
| try { | |
| foreach (var pkg in enumerable) | |
| { | |
| TryLoadPackageIntoLibrary(pkg); | |
| } | |
| } | |
| finally{ | |
| EngineController.DisableRun = false; | |
| } |
There was a problem hiding this comment.
It would at least be worthwhile checking that the exception caught inside TryLoadPackageIntoLibrary is not re-thrown from the Log methods?
There was a problem hiding this comment.
log doesn't seem to rethrow anything.
|
@reddyashish can you write a simple test that asserts the flag is updated to the correct state while packages load and then set to the correct state afterwards? You can likely just use the packageLoader tests for this. |
|
@reddyashish - does this change make the loading of packages faster? Since we are no longer executing the graph a number of times related to the number of custom functions present in the graph? |
|
Added the test. Please take a look |
|
@mjkkirschner It would definitely reduce the number of executions when the graph is in automatic mode. If the graph is in manual mode, there would be no difference as the run's are not triggered anyways. so I would say yes. Not sure if we can validate it using the performance benchmark. |
| /// A followup task is added https://jira.autodesk.com/browse/DYN-2120 to refactor the approach to this solution. | ||
| /// This test needs to be modified in that case. | ||
| [Test] | ||
| public void PackageLoadExceptionTest() |
There was a problem hiding this comment.
can you add category as well:
TechDebt
|
I think as it is if we cannot find a better solution this looks good as a patch fix for the regression - @aparajit-pratap have you had a chance to consider alternatives?, I am also looking but nothing lower risk has turned up yet. |
reddyashish
changed the title
|
Merging this |
horatiubota
added
the
error/warning/crash
Purpose
This PR is to cherry-pick #9957 into RC2.4.0_master branch.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@aparajit-pratap @mjkkirschner @QilongTang