Returning Autodesk.Revit.DB.Document results in crash if the document is lost

[johnpierson]

Dynamo version

1.3.x, 2.x.x

Operating system

Windows 10

What did you do?

This is technically a Revit thing, but I think the scope is beyond that..

When you return an Autodesk.Revit.DB.Document from a custom zero touch node, you will receive a nasty crash if the document is lost (fairly typical if you use background opened document workflows) and Dynamo is in automatic run mode.

FWIW I "fixed it" with the string fix I mention below. But this basically made it to where my nodes only work with each other. lol

What would you like to see?

I was curious if there is anyway to prevent the error that results. I have thought about several different ways to do this from view extensions to remove the connection or force to manual run mode. I finally landed on storing the document internally and returning the string name but that isn't great as some nodes require an Autodesk.Revit.DB.Document .

What did you see instead?

Object reference not set to an instance of an object.

   at ProtoCore.DSASM.Heap.Sweep()
   at ProtoCore.DSASM.Heap.SingleStep(Boolean forceGC)
   at ProtoCore.DSASM.Heap.FullGC(IEnumerable`1 gcroots, Executive exe)
   at ProtoScript.Runners.LiveRunner.ApplyUpdate()
   at ProtoScript.Runners.LiveRunner.CompileAndExecuteForDeltaExecution(List`1 astList)
   at ProtoScript.Runners.LiveRunner.SynchronizeInternal(GraphSyncData syncData)
   at ProtoScript.Runners.LiveRunner.UpdateGraph(GraphSyncData syncData)
   at Dynamo.Scheduler.UpdateGraphAsyncTask.HandleTaskExecutionCore()
   at Dynamo.Scheduler.AsyncTask.Execute()

Related Links:

https://forum.dynamobim.com/t/copy-views-to-document-on-file/33448 https://forum.dynamobim.com/t/close-background-revit-files/32837/10

[dimven]

This is why you usually need wrapper classes, like the one that's already in place (but very limited) for document objects:

https://github.com/DynamoDS/DynamoRevit/blob/master/src/Libraries/RevitNodes/Application/Document.cs

Ideally the wrapper would track the life-cycle of the document and invalidate itself if the document is lost? I think that the best approach would be to extend the existing document wrapper and add a public constructor for it, so that in the long term all nodes and packages can use it.

[johnpierson]

Yeah that would be my hope as well. I think for the time being (to get this at least partially solved sooner than later), I might have my nodes toggle to manual run mode on placement for document related workflows. 😒

[johnpierson]

I also, fully understand that what we are doing is definitely not what Dynamo was intended for. So I don't expect it to be perfect right off the bat 😂

[johnpierson]

Eh here is my temp fix. Force manual mode on placement of the "risky" nodes.

[ThomasMahon]

Your only solution would be to incorporate the close and save input into the document open node. That way the document object gets safely disposed. The exception thrown by Dynamo i suspect is due to the fact that the document object is both open and closed at the same time which leads to an inconsistent state. Since graphs are acyclic by nature (which therefore gives rise to the inconsistent state of the document object), I doubt there are many options available to resolve this without creating a special condition in Dynamo's VM to handle this exception. It may also explain why Dynamo's document wrapper class is static, which doesn't bode well for finding a solution that doesn't involve collating both open, close and save into one node. Just my 10 cents on the subject - its the workaround I used for CAD.ReportInstances in BimorphNodes.

[johnpierson]

Thanks for the comments @ThomasMahon. I agree with your points. I have actually implemented something similar to what you mentioned in my node that upgrades Revit files. It handles both open and close in the same node with no issues.

I was also handling this another way by storing the document in AppDomain with the title as the key. That resulted in no crashes as well since I was not returning the raw Autodesk.Revit.DB.Document but a string. The issue with that was, users inevitably wanted to use the Autodesk.Revit.DB.Document with other nodes and couldn't since my version was made to work with my nodes only because they were parsing the AppDomain.

FWIW, background open nodes are not something I have expanded upon once I saw these issues. But people were using the nodes already, so I didn't want to remove them. Specifically, OpenDocumentFile and CloseDocument. However, another user took it upon themselves to build a ton of background document nodes, so I may retire mine and send them his way for further support.

[ThomasMahon]

Seems sensible - an alternative, although still far from perfect, might be make the close document node asynchronous and to only execute on Dynamo's close event. If this is clearly documented in the node description, users will at least understand the document will remain open while Dynamo is still open (come to think of it, this behaviour could be implemented into the document open node). The immediate problem with this though....is when a user needs to close the document and do something else while in the same Dynamo session. I think any approach will be a compromise tbh, as the problem is intrinsically linked to the mechanics/limitations of visual programming (or at least, Dynamo's VP mechanics).

[johnpierson]

execute on Dynamo's close event

Oooh, that is a pretty nifty idea too. So the node would kind of "mark document for closing". Like you said, not perfect, but better than a hard crash. 👍

[ThomasMahon]

Yep exactly 👍

[mjkkirschner]

hmm @aparajit-pratap in any case - garbage collection should not crash should it when trying to collect a null?

[mjkkirschner]

@johnpierson - we're considering marking this won't fix - do you have any data (anecdotal or otherwise) on how often this occurs?

It also sounds like there is a larger DynamoRevit API redesign that you want to discuss here - where simply avoiding the crash might not actually help?

[johnpierson]

I think I basically need a wrapper for Document. 😁

On Mon, Nov 18, 2019 at 11:07 AM Michael Kirschner notifications@github.com wrote:

@johnpierson https://github.com/johnpierson - we're considering marking this won't fix - do you have any data (anecdotal or otherwise) on how often this occurs?

It also sounds like there is a larger DynamoRevit API redesign that you want to discuss here - where simply avoiding the crash might not actually help?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/DynamoDS/Dynamo/issues/9682?email_source=notifications&email_token=ADYD5VDEWXN3P7I46RAP6YLQULRXZA5CNFSM4HIMZQN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEELRZNY#issuecomment-555162807, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADYD5VFWRAMMGJ3SWDB4F4LQULRXZANCNFSM4HIMZQNQ .

[mjkkirschner]

tracked the crash with: https://jira.autodesk.com/browse/DYN-2287

[mjkkirschner]

I've transferred this to DynamoRevit because the request at this point seems to be to add a better wrapper for document which better tracks the lifecycle of the document object.

[mjkkirschner]

@QilongTang @wangyangshi @honghuiqin FYI - long standing issues here.

1. crashes occurring when document is actually deleted but wrapper still exists, as Dynamo GC tries to dispose it again or access properties on disposed object.
2. authors can't leverage the dynamo document wrapper type and have to all write their own wrappers and convert between them to make packages interoperate. https://www.linkedin.com/feed/update/urn:li:activity:6946172252740624384/

[erfajo]

@mjkkirschner, this goes also for Parameters, FamilyDocument, FamilyParameter, and many more classes! Why do you accept classes being shielded? We did not have any other chance than to create our own wrappers until I saw @johnpierson post... now we can circumvent the limitations... but why do we need to use reflection to solve a problem we never should have had!?

[mjkkirschner]

Hi @erfajo,

I think by shielded you mean internal or not using a public access modifier.

The reason we do not make all types public is for API compatibility and maintenance. We try to adhere to semantic versioning. https://semver.org/

We do not succeed entirely, and one reason is we do not have a clear boundary for what is an API. We have tried to improve this at least in DynamoCore over time.

The DynamoRevit project also tries to achieve this, though I am less confident in it, as you are probably aware, Revit itself breaks API compatibility yearly intentionally, Dynamo does not.

We anticipate our classes/types in DynamoCore.dll, DynamoServices.dll and ProtoGeometry.dll being used over a long time in many versions of Revit and other products, and in user extensions. That is one reason Dynamo is on version 2.16 and has not moved to a version 3.0 yet, moving to version 3.x would mean we would be able to break API compatibility. But it needs to happen all at once to satisfy sem ver, and it will be painful for developers and the ecosystem, so it must be timed carefully.

When we have made a type internal it's probably for a few reasons:

1. we were not sure what the consequences would be if we made it public - so we were not sure we could support it.
2. we were not sure it was the right design, and want to be able to change it without moving to a new major version number, since API changes should all happen at once then.
3. 1 or 2 were true, and then we forgot to come back, evaluate and make it public, and moved on to other things.

I think the best course of action for this type of problem is to push hard for the API to be expanded, use workarounds like reflection only temporarily until the API is expanded. Send a PR if you are able making the type public and explain why you are making it public, or why you need the API to be made public by the teams.

Reflection is great, but when internal signatures change (which they will, that is why they are internal) - reflection will break at runtime.