Move RCTRootShadowView creation to main thread

[Adam Gleitman (amgleitman)]

Please select one of the following

• I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
• I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Summary

Solves the same problem that #733 was supposed to solve (originally backed out by 70ddf46).

We've seen a deadlock on iOS with two threads, one main and one background, making simultaneous calls to +[RCTI18nUtil sharedInstance], with a background thread reaching the inside of the dispatch_once block (as seen here) first. It appears that playing around with NSUserDefaults on iOS (as done here) requires use of the main thread to "commit" changes to user defaults.

The solution is to move the initialization of RCTRootShadowView, which depends on our RCTI18n singleton as per this line, to the main thread, and do everything else on the UIManager queue.

This will later be brought upstream.

Changelog

[iOS] [Fixed] - Remove a deadlock involving RCTI18nUtil

Test Plan

The deadlock no longer occurs on iOS, and macOS appears unaffected.

[Harrie Shin (harrieshin)]

do you know which commit actually regress it in .68 version?

[Harrie Shin (harrieshin)]

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

[Harrie Shin (harrieshin)]

should we add an asserts in RCTI18n to make sure some things are called in main thread?

[Saad Najmi (Saadnajmi)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

[Adam Gleitman (amgleitman)]

Harrie Shin (@harrieshin)

do you know which commit actually regress it in .68 version?

Not sure. I haven't looked too deeply into it.

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

Correct. This is a separate thread.

should we add an asserts in RCTI18n to make sure some things are called in main thread?

I considered it, although I don't know if this will have any other adverse side effects, and I want to make sure that requiring main-thread initialization is indeed correct. If this ends up being the case, I'm happy to add an assert in.

[Adam Gleitman (amgleitman)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

The point of this change is to bump up the initialization of our RCTI18nUtil instance to avoid deadlocks. I'd like to think that this change is more surgical than the previous one, but I understand your concern. I can test the scenario that caused the reversion to make sure that still works as intended though.

[Adam Gleitman (amgleitman)]

Please select one of the following

• I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
• I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Summary

Solves the same problem that #733 was supposed to solve (originally backed out by 70ddf46).

We've seen a deadlock on iOS with two threads, one main and one background, making simultaneous calls to +[RCTI18nUtil sharedInstance], with a background thread reaching the inside of the dispatch_once block (as seen here) first. It appears that playing around with NSUserDefaults on iOS (as done here) requires use of the main thread to "commit" changes to user defaults.

The solution is to move the initialization of RCTRootShadowView, which depends on our RCTI18n singleton as per this line, to the main thread, and do everything else on the UIManager queue.

This will later be brought upstream.

Changelog

[iOS] [Fixed] - Remove a deadlock involving RCTI18nUtil

Test Plan

The deadlock no longer occurs on iOS, and macOS appears unaffected.

[Harrie Shin (harrieshin)]

do you know which commit actually regress it in .68 version?

[Harrie Shin (harrieshin)]

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

[Harrie Shin (harrieshin)]

should we add an asserts in RCTI18n to make sure some things are called in main thread?

[Saad Najmi (Saadnajmi)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

[Adam Gleitman (amgleitman)]

Harrie Shin (@harrieshin)

do you know which commit actually regress it in .68 version?

Not sure. I haven't looked too deeply into it.

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

Correct. This is a separate thread.

should we add an asserts in RCTI18n to make sure some things are called in main thread?

I considered it, although I don't know if this will have any other adverse side effects, and I want to make sure that requiring main-thread initialization is indeed correct. If this ends up being the case, I'm happy to add an assert in.

[Adam Gleitman (amgleitman)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

The point of this change is to bump up the initialization of our RCTI18nUtil instance to avoid deadlocks. I'd like to think that this change is more surgical than the previous one, but I understand your concern. I can test the scenario that caused the reversion to make sure that still works as intended though.

[Adam Gleitman (amgleitman)]

Please select one of the following

• I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
• I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Summary

Solves the same problem that #733 was supposed to solve (originally backed out by 70ddf46).

We've seen a deadlock on iOS with two threads, one main and one background, making simultaneous calls to +[RCTI18nUtil sharedInstance], with a background thread reaching the inside of the dispatch_once block (as seen here) first. It appears that playing around with NSUserDefaults on iOS (as done here) requires use of the main thread to "commit" changes to user defaults.

The solution is to move the initialization of RCTRootShadowView, which depends on our RCTI18n singleton as per this line, to the main thread, and do everything else on the UIManager queue.

This will later be brought upstream.

Changelog

[iOS] [Fixed] - Remove a deadlock involving RCTI18nUtil

Test Plan

The deadlock no longer occurs on iOS, and macOS appears unaffected.

[Harrie Shin (harrieshin)]

do you know which commit actually regress it in .68 version?

[Harrie Shin (harrieshin)]

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

[Harrie Shin (harrieshin)]

should we add an asserts in RCTI18n to make sure some things are called in main thread?

[Saad Najmi (Saadnajmi)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

[Adam Gleitman (amgleitman)]

Harrie Shin (@harrieshin)

do you know which commit actually regress it in .68 version?

Not sure. I haven't looked too deeply into it.

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

Correct. This is a separate thread.

should we add an asserts in RCTI18n to make sure some things are called in main thread?

I considered it, although I don't know if this will have any other adverse side effects, and I want to make sure that requiring main-thread initialization is indeed correct. If this ends up being the case, I'm happy to add an assert in.

[Adam Gleitman (amgleitman)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

The point of this change is to bump up the initialization of our RCTI18nUtil instance to avoid deadlocks. I'd like to think that this change is more surgical than the previous one, but I understand your concern. I can test the scenario that caused the reversion to make sure that still works as intended though.

[Adam Gleitman (amgleitman)]

Please select one of the following

• I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
• I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Summary

Solves the same problem that #733 was supposed to solve (originally backed out by 70ddf46).

We've seen a deadlock on iOS with two threads, one main and one background, making simultaneous calls to +[RCTI18nUtil sharedInstance], with a background thread reaching the inside of the dispatch_once block (as seen here) first. It appears that playing around with NSUserDefaults on iOS (as done here) requires use of the main thread to "commit" changes to user defaults.

The solution is to move the initialization of RCTRootShadowView, which depends on our RCTI18n singleton as per this line, to the main thread, and do everything else on the UIManager queue.

This will later be brought upstream.

Changelog

[iOS] [Fixed] - Remove a deadlock involving RCTI18nUtil

Test Plan

The deadlock no longer occurs on iOS, and macOS appears unaffected.

[Harrie Shin (harrieshin)]

do you know which commit actually regress it in .68 version?

[Harrie Shin (harrieshin)]

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

[Harrie Shin (harrieshin)]

should we add an asserts in RCTI18n to make sure some things are called in main thread?

[Saad Najmi (Saadnajmi)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

[Adam Gleitman (amgleitman)]

Harrie Shin (@harrieshin)

do you know which commit actually regress it in .68 version?

Not sure. I haven't looked too deeply into it.

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

Correct. This is a separate thread.

should we add an asserts in RCTI18n to make sure some things are called in main thread?

I considered it, although I don't know if this will have any other adverse side effects, and I want to make sure that requiring main-thread initialization is indeed correct. If this ends up being the case, I'm happy to add an assert in.

[Adam Gleitman (amgleitman)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

The point of this change is to bump up the initialization of our RCTI18nUtil instance to avoid deadlocks. I'd like to think that this change is more surgical than the previous one, but I understand your concern. I can test the scenario that caused the reversion to make sure that still works as intended though.

[Adam Gleitman (amgleitman)]

Please select one of the following

• I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
• I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Summary

Solves the same problem that #733 was supposed to solve (originally backed out by 70ddf46).

We've seen a deadlock on iOS with two threads, one main and one background, making simultaneous calls to +[RCTI18nUtil sharedInstance], with a background thread reaching the inside of the dispatch_once block (as seen here) first. It appears that playing around with NSUserDefaults on iOS (as done here) requires use of the main thread to "commit" changes to user defaults.

The solution is to move the initialization of RCTRootShadowView, which depends on our RCTI18n singleton as per this line, to the main thread, and do everything else on the UIManager queue.

This will later be brought upstream.

Changelog

[iOS] [Fixed] - Remove a deadlock involving RCTI18nUtil

Test Plan

The deadlock no longer occurs on iOS, and macOS appears unaffected.

[Harrie Shin (harrieshin)]

do you know which commit actually regress it in .68 version?

[Harrie Shin (harrieshin)]

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

[Harrie Shin (harrieshin)]

should we add an asserts in RCTI18n to make sure some things are called in main thread?

[Saad Najmi (Saadnajmi)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

[Adam Gleitman (amgleitman)]

Harrie Shin (@harrieshin)

do you know which commit actually regress it in .68 version?

Not sure. I haven't looked too deeply into it.

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

Correct. This is a separate thread.

should we add an asserts in RCTI18n to make sure some things are called in main thread?

I considered it, although I don't know if this will have any other adverse side effects, and I want to make sure that requiring main-thread initialization is indeed correct. If this ends up being the case, I'm happy to add an assert in.

[Adam Gleitman (amgleitman)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

The point of this change is to bump up the initialization of our RCTI18nUtil instance to avoid deadlocks. I'd like to think that this change is more surgical than the previous one, but I understand your concern. I can test the scenario that caused the reversion to make sure that still works as intended though.

[Adam Gleitman (amgleitman)]

Please select one of the following

• I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
• I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Summary

Solves the same problem that #733 was supposed to solve (originally backed out by 70ddf46).

We've seen a deadlock on iOS with two threads, one main and one background, making simultaneous calls to +[RCTI18nUtil sharedInstance], with a background thread reaching the inside of the dispatch_once block (as seen here) first. It appears that playing around with NSUserDefaults on iOS (as done here) requires use of the main thread to "commit" changes to user defaults.

The solution is to move the initialization of RCTRootShadowView, which depends on our RCTI18n singleton as per this line, to the main thread, and do everything else on the UIManager queue.

This will later be brought upstream.

Changelog

[iOS] [Fixed] - Remove a deadlock involving RCTI18nUtil

Test Plan

The deadlock no longer occurs on iOS, and macOS appears unaffected.

[Harrie Shin (harrieshin)]

do you know which commit actually regress it in .68 version?

[Harrie Shin (harrieshin)]

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

[Harrie Shin (harrieshin)]

should we add an asserts in RCTI18n to make sure some things are called in main thread?

[Saad Najmi (Saadnajmi)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

[Adam Gleitman (amgleitman)]

Harrie Shin (@harrieshin)

do you know which commit actually regress it in .68 version?

Not sure. I haven't looked too deeply into it.

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

Correct. This is a separate thread.

should we add an asserts in RCTI18n to make sure some things are called in main thread?

I considered it, although I don't know if this will have any other adverse side effects, and I want to make sure that requiring main-thread initialization is indeed correct. If this ends up being the case, I'm happy to add an assert in.

[Adam Gleitman (amgleitman)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

The point of this change is to bump up the initialization of our RCTI18nUtil instance to avoid deadlocks. I'd like to think that this change is more surgical than the previous one, but I understand your concern. I can test the scenario that caused the reversion to make sure that still works as intended though.

[Adam Gleitman (amgleitman)]

Please select one of the following

• I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
• I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Summary

Solves the same problem that #733 was supposed to solve (originally backed out by 70ddf46).

We've seen a deadlock on iOS with two threads, one main and one background, making simultaneous calls to +[RCTI18nUtil sharedInstance], with a background thread reaching the inside of the dispatch_once block (as seen here) first. It appears that playing around with NSUserDefaults on iOS (as done here) requires use of the main thread to "commit" changes to user defaults.

The solution is to move the initialization of RCTRootShadowView, which depends on our RCTI18n singleton as per this line, to the main thread, and do everything else on the UIManager queue.

This will later be brought upstream.

Changelog

[iOS] [Fixed] - Remove a deadlock involving RCTI18nUtil

Test Plan

The deadlock no longer occurs on iOS, and macOS appears unaffected.

[Harrie Shin (harrieshin)]

do you know which commit actually regress it in .68 version?

[Harrie Shin (harrieshin)]

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

[Harrie Shin (harrieshin)]

should we add an asserts in RCTI18n to make sure some things are called in main thread?

[Saad Najmi (Saadnajmi)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

[Adam Gleitman (amgleitman)]

Harrie Shin (@harrieshin)

do you know which commit actually regress it in .68 version?

Not sure. I haven't looked too deeply into it.

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

Correct. This is a separate thread.

should we add an asserts in RCTI18n to make sure some things are called in main thread?

I considered it, although I don't know if this will have any other adverse side effects, and I want to make sure that requiring main-thread initialization is indeed correct. If this ends up being the case, I'm happy to add an assert in.

[Adam Gleitman (amgleitman)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

The point of this change is to bump up the initialization of our RCTI18nUtil instance to avoid deadlocks. I'd like to think that this change is more surgical than the previous one, but I understand your concern. I can test the scenario that caused the reversion to make sure that still works as intended though.

[Adam Gleitman (amgleitman)]

Please select one of the following

• I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
• I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
• I am making a fix / change for the macOS implementation of react-native
• I am making a change required for Microsoft usage of react-native

Summary

Solves the same problem that #733 was supposed to solve (originally backed out by 70ddf46).

We've seen a deadlock on iOS with two threads, one main and one background, making simultaneous calls to +[RCTI18nUtil sharedInstance], with a background thread reaching the inside of the dispatch_once block (as seen here) first. It appears that playing around with NSUserDefaults on iOS (as done here) requires use of the main thread to "commit" changes to user defaults.

The solution is to move the initialization of RCTRootShadowView, which depends on our RCTI18n singleton as per this line, to the main thread, and do everything else on the UIManager queue.

This will later be brought upstream.

Changelog

[iOS] [Fixed] - Remove a deadlock involving RCTI18nUtil

Test Plan

The deadlock no longer occurs on iOS, and macOS appears unaffected.

[Harrie Shin (harrieshin)]

do you know which commit actually regress it in .68 version?

[Harrie Shin (harrieshin)]

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

[Harrie Shin (harrieshin)]

should we add an asserts in RCTI18n to make sure some things are called in main thread?

[Saad Najmi (Saadnajmi)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

[Adam Gleitman (amgleitman)]

Harrie Shin (@harrieshin)

do you know which commit actually regress it in .68 version?

Not sure. I haven't looked too deeply into it.

for my own knowledge, RCTExecuteOnUIManagerQueue is not main thread?

Correct. This is a separate thread.

should we add an asserts in RCTI18n to make sure some things are called in main thread?

I considered it, although I don't know if this will have any other adverse side effects, and I want to make sure that requiring main-thread initialization is indeed correct. If this ends up being the case, I'm happy to add an assert in.

[Adam Gleitman (amgleitman)]

The last time we had a deadlock in RCTI18nUtil, I made a change in both RN-macOS and RN Core to resolve it. The change ended up being incorrect, and I had to back it out. The actual fix was downstream in our internal codebase. I worry that that sequence of events may happen again here. Are we sure there isn't something we can change downstream instead of making the change here?

The point of this change is to bump up the initialization of our RCTI18nUtil instance to avoid deadlocks. I'd like to think that this change is more surgical than the previous one, but I understand your concern. I can test the scenario that caused the reversion to make sure that still works as intended though.