Fix: await device.lost hangs when called before device.destroy()
#129
Conversation
|
@hmallen99 I'm very excited about this. Right now I'm heads down with some deep Canvas API refactorings but after that I could look into the nitro module migration. |
Migrating to nitro modules would be ideal! I think this should act as a stop-gap until that migration can happen, though. I've tested this against the conformance test suite, and it removes a lot of the blockers for running the tests. The remaining inventory seems to be around throwing the proper |
|
I agree! Should we jump on a call to discuss how to run CTS with this module? I would love to learn more. |
|
Yes, let's hop on a call @wcandillon! Please let me know how and when you would like to call. The process of running tests requires a bit of setup right now, but I can walk you through some of it. It's fairly similar to running the example app in this repo. I'm hoping to make it more automated in the future, though. |
|
I also updated the README to include build and test instructions in my fork of the cts, if you want to give that a try. It would be useful to have the feedback if those instructions don’t make sense! https://github.com/rebeckerspecialties/gpuweb-cts/blob/main/README.md |
|
my email is public, can you send me a proposal calendar invite? |
Sent you an invite! |
…-native-webgpu into fix/async-future
|
Hey @wcandillon, are we able to merge this PR? It should be working on both iOS and android with tests passing. Thanks! |
|
thanks for letting me know, I will start reviewing it. |
Hmm, I'll take a look at the CI failures. Thanks for running the actions! |
…-native-webgpu into fix/async-future
|
@hmallen99 we're on the verge of merging #137, this PR will be next immediately after that 🙏 |
Awesome, I just validated that all the static analysis and test steps are passing locally. I'll validate the |
|
@hmallen99 just had a look and it makes sense . Did you try it on Android? I don't think we pass the jsInvoker there. |
|
@hmallen99 I can fix the android issue for you |
Thanks @wcandillon, that would be great! The android simulator seems to run very slowly on my machine, so it's hard for me to debug. |
Resolves #126
Problem
Fixes a bug where javascript promises will hang the main thread while waiting for an
std::futurevalue on the native side. In the case ofawait device.lost, this causes the main thread to hang indefinitely, asdevice.destroycannot be called to resolve the promise. This bug was discovered by running the impl against the gpuweb cts.Solution
std::futureto JS promise to use a more robust async implementation, allowing promises to await on a background thread..hppfiles have been converted to.hfiles and the namespace has been shortened frommargelo::nitro->margeloto match the conventions used in this repo.Testing
Devicetest suite to validate that device.lost will not hang the main thread if device.destroy is not called. With the old threading logic, this test hangs indefinitely. With the new test logic, we are able to cancel the promise.device.lostresolves if it is awaited before callingdevice.destroy