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

Memory leak due to React state update on unmounted components #74

Open
thai-truong opened this issue Jun 24, 2020 · 3 comments
Open

Memory leak due to React state update on unmounted components #74

thai-truong opened this issue Jun 24, 2020 · 3 comments
Labels
bug Something isn't working low priority Will work on when we have time

Comments

@thai-truong
Copy link
Collaborator

thai-truong commented Jun 24, 2020

Navigating to different pages of the portal very fast using the navbar will result in memory leak errors being printed to the console, due to React state update being done on unmounted components. This only happens to pages that are listed as tabs on the navbar.

An example of the error message:

Warning: Can't perform a React state update on an unmounted component. This is a no-op, but it indicates a memory leak in your application. To fix, cancel all subscriptions and asynchronous tasks in the componentWillUnmount method.
in CalendarPage (created by WithStyles(CalendarPage))
in WithStyles(CalendarPage) (at RoutingByContextAuth.js:17)
in main (at NavBar/index.js:177)
in div (at NavBar/index.js:126)
in NavBar (created by WithStyles(NavBar))
in WithStyles(NavBar) (at RoutingByContextAuth.js:16)
in Unknown (created by Context.Consumer)

Temporary fix is to use this._isMounted in the following manner:
- Set this._isMounted to false in the constructor of the class component
- Set this._isMounted to true in componentDidMount()
- Set this._isMounted to false in componentWillUnmount()
- Look for any this.setState() and do
if(this._isMounted) this.setState(...);

A cleaner and more permanent fix is preferred.

@thai-truong thai-truong added the bug Something isn't working label Jun 24, 2020
@thai-truong thai-truong added this to the Sprint 1: Tofu milestone Jun 24, 2020
@godwinpang godwinpang added the low priority Will work on when we have time label Jul 4, 2020
@godwinpang godwinpang removed this from the Sprint 1: Tofu milestone Jul 4, 2020
@godwinpang
Copy link
Collaborator

This might take a while; let's put it on the backburner for now

@godwinpang
Copy link
Collaborator

@thaigillespie might be worthwhile to talk a little about bluebird here

@thai-truong
Copy link
Collaborator Author

thai-truong commented Aug 6, 2020

This issue is mainly caused by calling this.setState() in Promise chains (like in .then()) in functions like componentDidMount(). This is because if the Promise (chain) is still pending until or after a component unmounts, then the Promise (chain) resolves and .setState() gets called, then since the component is already unmounted, we get a memory leak error in the console. Navigating too quickly between pages causes the error detailed above in the issue description because the promise (with .setState()) from a component gets resolved AFTER the component unmounts, since you're moving too fast between pages, it's not resolving quickly enough.

A solution to this is cancellable promise using Bluebird [(http://bluebirdjs.com/docs/api/cancellation.html)]. Not sure if we have to change all of our promises to using Bluebird promises in order to be able to do this but yeah this will be very useful.

@godwinpang godwinpang added wontfix This will not be worked on and removed wontfix This will not be worked on labels Aug 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working low priority Will work on when we have time
Projects
None yet
Development

No branches or pull requests

2 participants