microsoft / react-native-windows

A framework for building native Windows apps with React.
https://microsoft.github.io/react-native-windows/
Other
16.39k stars 1.14k forks source link

When is it safe to access JsiRuntime (need clear error and better documentation) #7374

Open asklar opened 3 years ago

asklar commented 3 years ago

Currently Chakra does not allow accessing the jsi from a thread other than the JS thread. It's unclear whether the rest of the runtimes have the same restriction or not. You can post a lambda to the m_reactContext.JSDispatcher(), we should document it beyond the API reference. There should be diagnostics and docs for this, currently trying to access the runtime from the ui thread will terminate the app (instead of sending the exception back to app code)

It is also not clear that as a view manager e.g. it is not safe to try to access the jsi runtime from e.g. GetNativeProps - the runtime won't have been created yet

NickGerleman commented 3 years ago

Currently Chakra does not allow accessing the jsi from a thread other than the JS thread. It's unclear whether the rest of the runtimes have the same restriction or not.

They will. The JSI model allows synchronous/chatty calls between native and JS. Because they are synchronous, doing a thread hop would need to be async or involve COM STA style thread blocking.

There is better reference in the C++ MSRN JSI headers. Note that the ABI JsiRuntime type is not meant to be the user API for C++. Look at JsiApiContext.h for what users should be calling.

currently trying to access the runtime from the ui thread will terminate the app (instead of sending the exception back to app code)

When you say sending an exception to app code, do you mean C++ exception, redbox, instance error? We often intentionally crash on failures of internal invariants, which I think is fine, but wonder if there are better ways to deliver user signal as to what invariant was incorrect.

NickGerleman commented 3 years ago

Re docs, I think there was back-and-forward on whether we wanted to document this yet. Though I guess we have a lesson from here and Discord though that it's hard to signal what APIs are at what level of stability. Documentation as experimental might be better than no docs.

asklar commented 3 years ago

The documentation question is a bit off topic, but if an API is exposed through WinMD or one of the Cxx headers, it must be documented before release. It's fine to mark it as experimental, we still need to document it though.

When you say sending an exception to app code, do you mean C++ exception, redbox, instance error? We often intentionally crash on failures of internal invariants, which I think is fine, but wonder if there are better ways to deliver user signal as to what invariant was incorrect.

Yes, either/or. Right now the app terminates immediately without a chance for app code to recover/test if it is ok to call these APIs.

NickGerleman commented 3 years ago

I think termination when thread afined functions are called on the wrong thread is probably for the best. There's value in screaming loudly instead of offering an error code and path to recovery. This pattern is followed today for many UI thread affined APIs for example. Exceptions could work well here, but much of our code is intentionally noexcept.

Need to be clear what we're screaming loudly about though. I.e. make sure the right signal is provided when we crash.

chrisglein commented 3 years ago

Actions I'm hearing here:

vmoroz commented 3 years ago

We must not access the JSI internals directly. Instead, developers must use ExecuteJsi method that takes care about accessing JSI from the JSDispatcher even if it was called from a different thread.