-
Notifications
You must be signed in to change notification settings - Fork 30
fix(rhino-importer): Use main thread always for document creation #1161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1161 +/- ##
=======================================
Coverage 41.93% 41.93%
=======================================
Files 113 113
Lines 3207 3207
Branches 314 314
=======================================
Hits 1345 1345
Misses 1819 1819
Partials 43 43 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| try | ||
| { | ||
| // This needs to be called on the main thread | ||
| importer = factory.Create(importerArgs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main change. We're opening the document here on construction of the importer.
This way, we're doing it on the main thread.
Previously, this was done from a .NET worker thread inside the Task.Run block lower down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would we ever need to run things on a worker thread in an importer environment?
re your PR desc, even if McNeel fixes, is not the safe to run always on main?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. There's another issue with Rhino inside where, as soon as the main thread is yielded (via the await keyword), It will be hogged for ever by something internal to Rhino.
This means, once an await keyword is used, anywhere, then we've lost control over the main thread now for the remaining process life span.
This means we have to be careful to use the main thread first, and then use async after.
That's what this PR achieves. but it's working around several weird behaviours of Rhino Inside.
I get the impression that Rhino Inside wasn't really built to be very compatible with await async, since none of the examples from mcneel involve async.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sense to me, got it thanks!
I hope i am not annoying but what stop us to run everything sync?
Occasionally, we're seeing
OpenHeadlesscrash the importer with the following errorThis was leading to many 3dm imports failing with the error message "
Importer left an invalid response"OpenHeadlessappears to behave un-reliably when used from not the main thread. No idea why we're only noticing this now, could be some async stuff we've tweaked, could be an update to Rhino, not sure.I'm pretty sure this is a bug, and Dale from mcneel has tracked it - https://discourse.mcneel.com/t/rhino-inside-fatal-app-crashes-when-disposing-headless-documents/208673/6
For the mean time, I believe we can work around the issue by ensuring we're always calling
OpenHeadlessfrom the main thread. This isn't super easy to pull off, because Rhino will also hog the main thread if it's ever yielded, so this can only be done before anyawaitkeyword is used.And there's no compile time safety here, we just have to document and remember not to do any async stuff until after opening the document.