Skip to content
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

Is sharing ReactInstanceManager across multiple activities broken? #6410

Closed
guyca opened this issue Mar 10, 2016 · 20 comments
Closed

Is sharing ReactInstanceManager across multiple activities broken? #6410

guyca opened this issue Mar 10, 2016 · 20 comments
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.

Comments

@guyca
Copy link

guyca commented Mar 10, 2016

I'm trying to share a ReactInstanceManager singleton across multiple activities. I've wired my ReactInstanceManager instance the my activities life cycle methods, so when ReactActivity.onDestroy is called, the mReactInstanceManager also gets destroyed and then mCurrentReactContext is set to null.

When opening a new activity and instanciating react views with the singelton manager everything works fine. But when I close the second activity with the back button, it's onDestroy() is invoked after onResume() of the first activity. Now when I try to interact with react views i get the following warning message:

03-10 13:35:33.164 1627-1627/com.awesomeproject W/unknown:React: Unable to handle touch in JS as the catalyst instance has not been attached

since mReactInstanceManager.getCurrentReactContext() in handleTouchEvent() returns null.

Is sharing ReactInstanceManager across activities/fragment still supported?

I'm running RN 0.20.0 on Android.
The repro scenario is very simple:
create a react activity (MainActivity). Open another activity from it, and click the back button to go back to the MainActivity.
results: any RN view in MainActivity can't be interacted with.

Logs:

03-10 10:46:10.043 10740-10740/com.awesomeproject V/RctActivity[MainActivity]: onCreate
03-10 10:46:10.101 10740-10740/com.awesomeproject V/RctActivity[MainActivity]: onStart
03-10 10:46:10.101 10740-10740/com.awesomeproject V/RctActivity[MainActivity]: onResume
// Opening second activity
03-10 10:46:48.320 10740-10740/com.awesomeproject V/RctActivity[MainActivity]: onPause
03-10 10:46:48.325 10740-10740/com.awesomeproject V/RctActivity[SecondActivity]: onCreate
03-10 10:46:48.328 10740-10740/com.awesomeproject V/RctActivity[SecondActivity]: onStart
03-10 10:46:48.328 10740-10740/com.awesomeproject V/RctActivity[SecondActivity]: onResume
03-10 10:46:48.771 10740-10740/com.awesomeproject V/RctActivity[MainActivity]: onStop
// Back button clicked
03-10 10:47:29.772 10740-10740/com.awesomeproject V/RctActivity[SecondActivity]: onPause
03-10 10:47:29.775 10740-10740/com.awesomeproject V/RctActivity[MainActivity]: onStart
03-10 10:47:29.775 10740-10740/com.awesomeproject V/RctActivity[MainActivity]: onResume
03-10 10:47:30.105 10740-10740/com.awesomeproject V/RctActivity[SecondActivity]: onStop
03-10 10:47:30.105 10740-10740/com.awesomeproject V/RctActivity[SecondActivity]: onDestroy
@AndrewJack
Copy link
Contributor

Removing the calls to ReactInstanceManager.onDestroy() from any activities will prevent the instance manager from being destroyed.

I have moved the call to ReactInstanceManager.onDestroy to the Application's onTerminate() method.
This approach is working for me so far. Would be great to know if I should be doing it anywhere else.

This occurs because onResume sets the current activity, but onDestroy is called after this (when going back) which then removes the reference.

@guyca
Copy link
Author

guyca commented Mar 11, 2016

@AndrewJack My activities extend ReactActivity, as intended, in this case you can't really prevent the call to ReactActivity.onDestroy().

Seems like this issue was introduced when Add a base activity for React Native apps was merged.

@AndrewJack
Copy link
Contributor

Just make your own ReactActivity

@guyca
Copy link
Author

guyca commented Mar 12, 2016

hmm... I feel like that misses the whole point of having a base activity for react native apps.

@guyca
Copy link
Author

guyca commented Mar 12, 2016

@AndrewJack I wasn't aware of this, but according to the docs onTerminate is called only on simulator.

This method is for use in emulated process environments. It will
never be called on a production Android device, where processes are
removed by simply killing them; no user code (including this callback)
is executed when doing so.

Also ReactInstanceManager has a ReactContext which in turn, keeps a hard reference to the Activity (in ReactContext) so not calling onDestroy will result in memory leaks.

@satya164
Copy link
Contributor

@guyca You can just override the onDestroy method of ReactActivity to prevent destroying the instance manager.

@guyca
Copy link
Author

guyca commented Mar 12, 2016

@satya164 Thanks for the response. I might be experiencing a brain fart, but how does overriding onDestroy solves this? I would still have to call super.onDestroy

@satya164
Copy link
Contributor

@guyca Yeah, missed that.

@AndrewJack
Copy link
Contributor

@guyca Interesting about onTerminate. Once the process is destroyed everything is destroyed, so unless there is something major we need to tear down this shouldn't be an issue.

onResume sets the current activity, in ReactInstanceManagerImpl & ReactContext

This wouldn't cause a leak as long as every new activity calls onResume, which will set a new current activity. However, if an activity started that didn't call onResume, and the previous activity was destroyed then that activity would leak.

Note: onResume has been renamed to onHostResume in 0.22.

@guyca
Copy link
Author

guyca commented Mar 14, 2016

@satya164 are there any planes to address this issue? Should I make a pull request?

@satya164
Copy link
Contributor

@guyca Sure. Ping me when you do.

@guyca
Copy link
Author

guyca commented Mar 14, 2016

@satya164 wonderful, will do.

guyca added a commit to guyca/react-native that referenced this issue Mar 14, 2016
guyca added a commit to guyca/react-native that referenced this issue Mar 14, 2016
@guyca
Copy link
Author

guyca commented Mar 14, 2016

@satya164 here's my take on fixing this issue. What do you think?

@mkonicek
Copy link
Contributor

Just make your own ReactActivity

I agree.

ReactInstanceManager singleton across multiple activities

This seems like a scenario not many people run into (so far). Until it becomes common I don't think we need to support it in the base class.

@mkonicek
Copy link
Contributor

@facebook-github-bot answered

@facebook-github-bot
Copy link
Contributor

Closing this issue as @mkonicek says the question asked has been answered. Please help us by asking questions on StackOverflow. StackOverflow is amazing for Q&A: it has a reputation system, voting, the ability to mark a question as answered. Because of the reputation system it is likely the community will see and answer your question there. This also helps us use the GitHub bug tracker for bugs only.

@facebook-github-bot facebook-github-bot added the Ran Commands One of our bots successfully processed a command. label Mar 20, 2016
@sneerin
Copy link

sneerin commented Dec 1, 2017

We have the same issue

1 similar comment
@littlehome-eugene
Copy link

We have the same issue

@KingAlen
Copy link

ReactInstanceManager singleton across multiple activities
+1 @mkonicek

@yurykorzun
Copy link
Contributor

Still an issue and no clear solution to it.

@facebook facebook locked as resolved and limited conversation to collaborators May 24, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Ran Commands One of our bots successfully processed a command. Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

10 participants