isMounted is not enough to know if you can setState

[sbrandwoo]

For the sake of this, I will simplify the workflow into three stages:

1. The component is mounting, you may call setState, isMounted is false
2. The component is mounted, you may call setState, isMounted is true
3. The component is unmounted, you must not call setState, isMounted is false

The particular case I'm worried about is when you trigger a promise (or similar delayed action) from componentWillMount:

    componentWillMount: function () {
        doSomething(props).then(function (url) {
            if (this.isMounted()) {
                this.setState({url: url});
            }
        }.bind(this));
    }

The .then() could be called in two circumstances, synchronously in the same call stack, or asynchronously in a later stack (I know, mixing async and sync in the same signature is a bad idea, but for the sake of argument).

In the asynchronous case: the promise above could resolve at any time in the future, so in order to prevent calling setState when we aren't allowed to, we add the isMounted() check. But what if the promise resolves while the component is still in the mounting state, outside of the componentWillMount stack?

• is it possible for this to happen? Is there a chance for other JS to run between componentWillMount and the component being mounted, or are they in the same cycle?
• if it can, what will happen? Will an error be shown?

In the synchronous case: we will enter the block still in the mounting state, which means isMounted() will be false and my state won't be set.

Depending on the answers to my async questions I can see two outcomes:

1. Async cannot return before the component is mounted: this is a minor inconvenience that can be solved by coding such that you know what's async and what's not.
2. Async can return before being mounted: there is no way to know if setState() is safe to call or not, as isMounted() is incomplete. The user has to manually track the state of the component and if it has been unmounted or not.

Thoughts? Have I invented an issue where none exists?

[dmitrig01]

Just to add to the confusion: in componentWillUnmount, isMounted() is true and you are not allowed to setState()

[YourDeveloperFriend]

+1 not to the original issue by to @dmitrig01's comment.

[vjeux]

We explicitly say in the documentation that the goal of isMounted is to guard against setState: http://facebook.github.io/react/docs/component-api.html#ismounted

So it's not right that it doesn't work in all cases

[YourDeveloperFriend]

Can I question the point of isMounted? It just adds a lot of boilerplate. Is it possible to just build the check into setState? Unless the call to isMounted is expensive, I don't see any advantage to making the developer explicitly check it all the time.

function setState(newState) {
  if(this.isMounted()) {
    // insert setState code
  }
}

[dmitrig01]

Or, a setStateSafe, which does just that?

[gaearon]

Yeah, isMounted check before setState is rather tedious IMO. I'd rather have setState returning boolean (false if it couldn't).

[YourDeveloperFriend]

+1 setStateSafe. However, since I would always use setStateSafe and would see no use for setState, then I'm not sure we need setState at all, so why not replace the function entirely?

Either way, setStateSafe will only work if we fix the isMounted during componentWillUnmount issue.

[sbrandwoo]

I imagine setStateSafe would use some internal logic to determine what's safe or not, rather than using isMounted. It could even just fail silently rather than throwing an error like it currently does.

There might be a use case for an unsafe setState, but I can't think of one - it's certainly the more unlikely case.

[dmitrig01]

I think in general, React has a pattern of blowing up in your face when you use it wrong (case in point, setState when a component is not mounted, but there are plenty of others) – which is in general good.

However, in some cases, if you know what you're doing, it would be nice to suppress these errors.

I would see the distinction as being - setState is used where the component is known to be mounted (which is probably most: e.g., onChange handlers of <input>, etc). setStateSafe is for asynchronous callbacks: timer, AJAX call, etc.

[jimfb]

@sbrandwoo My understanding is that you're supposed to use componentWillUnmount to clean up any timers, cancel async requests, etc. Failure to cleanup is often a code smell that indicates a memory leak or other even more severe bug. That said, in the case of an asynchronous callback, it's easy to imagine why it would be easier to just ignore the update rather than attempt to cancel the callback, and this is where isMounted could come in as a handy guard.

Once a component is unmounted, I think it is effectively dead. I don't think it can be re-mounted (at least until opaque prerendered components are supported). There really shouldn't be a reason to update it anymore, so doing so would strongly imply a bug.

Having said that, I don't see why you couldn't set the state on an unmounted component, and the expected behavior is that it would effectively be a noop. My intuition would be to remove this error check and make setState and forceUpdate be valid at any time, maybe be a warning if you have findbugs turned on.

I suspect @sebmarkbage has strong opinions on this topic.

[dmitrig01]

@jsfb to provide a concrete example of my use case, I have a "timer" component which counts down from 30 seconds to 0. I have one method to create an interval, and another to clear the interval. I call the method to clear the interval after the 30th tick and on componentWillUnmount. In the first case, I want to do a setState({ interval: null }), but in the second, I can't. There's no way that I know of to know whether it's safe to do that from within the method, without passing a parameter in.

[rickbeerendonk]

In setState and replaceState are more checks than isMounted(), see function validateLifeCycleOnReplaceState(instance) in ReactCompositeComponent.js

I wrote a mixin, but due to limited access to internals, I am not happy with the result: http://github.com/rickbeerendonk/react-mixin-safe-state-change

[sebmarkbage]

Yea, there's no reason for this error to exist other than enforcing that you properly clean up your components. We could just as well ignore the value. I think that we'll likely make this a warning or at least make sure that it doesn't break reconciliation if it throws.

I would like to deprecate isMounted(). It's only really used as an escape to by-pass this error. The error is there to help you. If the easiest way to avoid the error is to add this isMounted() check, then it circumvents the whole purpose of having the error in the first place. You're not fixing the root problem. And as we all know, we tend to take shortcuts whenever we can. In practice, these go unfixed.

There are three separate scenarios in play here:

1) Sometimes asynchronously and some times synchronously calling some method that might set state.

This is generally a really bad idea. This is a race condition that will act differently depending on the timing of your system, so testing won't reveal scenarios that an end user will hit. If you write in a completely immutable style, it's not as bad, but still considered bad practice.

If you have cached values, that you would like to use instead of causing two renders, you can at least build a separate synchronous API for that. E.g. in getInitialState: if (promise.hasCachedValue) return { value: promise.cachedValue }; It's better than violating the expectations of a Promise or async callback.

Additionally, in the current server rendering model, it's best practice to trigger async side-effects in componentDidMount since that will only fire on the client.

However, this is mostly a non-issue if (3) is solved since setState is currently still allowed in componentWillMount.

2) setState in componentWillUnmount as a way to unify resetting values and clearing asynchronous callbacks.

This pattern seems a bit unnecessary to me. Unmounting and and reaching completion is two different concepts and are usually different code paths. Even if they do the same thing, it might be worth separating them to make refactoring easier when one changes.

I don't see this as a major issue that we even need to error in production, we could warn in dev. It doesn't suffer from the same consequences as (3). So we could even drop the warning.

However, the question is: Is it more commonly an mistake or a legit pattern? If it's a legit pattern, then we should not warn since warnings are meant to be cleaned up. Excessive warnings creates apathy. However, even if it is legit, should we warn anyway because the majority case is a mistake? I'm not sure. I could go either way. I don't think that this is a common mistake but not sure.

3) Calling setState when a component is completely unmounted.

This is a strong indication that an asynchronous callback isn't being properly cleaned up. Unfortunately, mainstream JS APIs makes it very easy to avoid cleaning up hanging asynchronous callbacks.

One callback isn't a big deal. However, that callback hangs on to objects and intermediate callbacks, promises and subscriptions. If alot of your components do this, you will quickly run into memory issues.

This is a case of death by a thousand cuts. There is always a reason to avoid the complexity of cleanup logic, so you end up convincing yourself that it's ok for your special case. Then you run into GC and memory issues, and blame React for being slow. This is especially critical on mobile devices where memory issues can cause a browser to crash.

There are two types of callbacks. Infinite (or very long living) callbacks, such as event handlers that fire multiple times. There are also single fire callbacks such as Promises.

Promises are not as bad since they only resolve once before they can be cleaned up. However, they also live a long time. E.g. data requests can sometimes have very long timeouts to handle low-latency/bandwidth environments. The example above of a 30 second timeout is also an example of a long living promise. If your components frequently unmount and remount, you'll accumulate a lot of hanging promises and finally crash.

Therefore, it really bothers me that the JS community has adopted Promises at scale since it doesn't come with a cleanup mechanism. However, from a pragmatic point of view, we should probably allow this pattern to work since Promises are so common and make it very difficult to do clean up. You've been warned.

The big issue is that we can't really tell if the callback is coming from a Promise or a long living event. Failure to cleanup a long living event is a very common and dangerous mistake. So, how do we prevent that mistake from happening?

Even if the current callstack is coming from a Promise, that's no guarantee. That Promise could've fired a persistent event subscription that called setState. E.g. a Flux store.

Ultimately, I think that the solution is to build support for Promises in state. That way we can easily make an exception and ignore the warning if the Promise resolves late.

We should also build in support for Observables that make it easy to get cleanup for free.

We could easily ignore these setState calls but we'd need another way to ensure that you're not making common dangerous mistakes.

[shubik]

@sebmarkbage thanks for these points. But I think that you position React as a police to enforcing good practices (in your subjective opinion) by blowing up an app with throwing hard errors. IMO warning would be good enough. Thank you for consideration.

[jimfb]

@shubik React already fails pretty softly in a bunch of places that I believe should be fatal. It's a complicated balance of making it abundantly clear to a developer exactly when/where things go wrong, vs. failing quietly. I agree with @sebmarkbage that we need a way to ensure developers are not making common dangerous mistakes, and provide escape hatches (like isMounted) when developers know what they're doing. That said, my intuition would be that setting state on an unmounted component would be analogous to a noop, so I tend to favor removing this error.

[shubik]

@jsfb I completely agree that ideally you would ignore setState on unmounted component and (at a given log level) print a respective warning. I would expect a hard error when your library has an internal error situation, not when it's an expected (although not desirable in your opinion) use case.

[sebmarkbage]

We'll probably make this a warning. However, note that we treat React warnings as if they're errors, except with a different code path in the error case. I.e. you should strive to never have any warnings in your code base. Even with a warning, it's not a fully supported use case and risk breaking.

I would like to be able to fully support the use case, but what's the API? I don't know. How do we distinguish between the clearly bad case (hanging callback) and a valid (in your opinion) pattern?

[shubik]

@sebmarkbage async XHR is a bad case but also the most common case, probably more common than timeouts or intervals that some of the commenters talked about - but at the other hand, as I and others pointed out, we are forced to check isMounted before every setState to make sure React never errors out. As I said, my main argument is that it makes more sense to throw an error if it's an internal error of React (e.g. using wrong arguments on React methods, etc.), not because of executing an async callback.

In any case, thanks a lot for your time and for having a dialog with us about this.

[gaearon]

A common case for me is non-React (e.g. window.addEventListener) event handlers. Although I unbind them in componentWillUnmount, some of them still seem to come through and calling setState fails there without isMounted guard.

[sebmarkbage]

@gaearon that sounds like a bug either in your code or in React? Do you have a case that you're able to reproduce that we can take a look at?

[gaearon]

@sebmarkbage Yeah I'll see if I can extract it.

[sbrandwoo]

I have been told by a reliable source that componentWillMount, render and componentDidMount happen together, so there is no chance of a callback returning somewhere in the middle and being confused by isMounted.

So this issue now refers only to the setState when unmounted case.

[dmitrig01]

Don't want to pile on, but I think an additional issue (I can open a separate issue for it maybe?) is that isMounted is meant to be used as a guard against an unsafe setState, but during componentWillUnmount, isMounted is true but it is not safe to call setState.

It would seem to me that this is an issue regardless of whether or not you should be calling setState in componentWillUnmount – whatever the guard is for setState, it should in that case return the status of "should not set state" during componentWillUnmount

[mridgway]

I have a case where we are using side-loading (Flux stores) at multiple levels within the component tree. There is a callback for the change event at the root level as well as at a child level. The root node callback gets called first and causes a re-render where the child node will no longer be rendered and thus gets unmounted, but the callback for the child's change handler still gets called and proceeds to setState. The child setState call now warns because it will be unmounted, but there wasn't a chance to stop the callback from being called.

I feel like side-loading being a first class citizen in React will cause this kind of case to be more common.

[sophiebits]

@mridgway Does the child component unsubscribe from the store on unmount? If so, it shouldn't get event callbacks after unmount unless I'm missing something.

[mridgway]

It does unsubscribe in unmount, but the change event handlers are already executing in order that they were registered. By the time removeListener is called it is too late unless EventEmitter was smart enough to know whether a handler was removed from its list in the middle of handling an event.

[MattKunze]

Just ran into this today within a redux project - we're triggering actions in componentWillUnmount to unload some things, which triggers a state change and rerender. The state is bound in a HOC so we don't really have the opportunity to unsubscribe first, and as mentioned above a isMounted() check doesn't help.

The pattern is a little strange, I'll probably just rework it so things aren't triggered from the lifecycle event. But it would be nice if there was at least the option to get componentWillUnmount and setState to place nicely together.

[roman-cliqr]

Got into same situation as @mridgway. Asked a question on Stack Overflow

[gaearon]

Just ran into this today within a redux project - we're triggering actions in componentWillUnmount to unload some things, which triggers a state change and rerender. The state is bound in a HOC so we don't really have the opportunity to unsubscribe first, and as mentioned above a isMounted() check doesn't help.

File an issue in React Redux and maybe we can find a way to fix it.

[infolock]

I've had this issue so many times. there are a bunch of solutions I've read up on too. either way, @donabrams gave a great response to @roman-cliqr SO issue - its the best answer to this problem i've seen so far.

[roman-cliqr]

Thanks for the feedback and opinions guys. I followed the solution provided by @gaearon on SO

[mmerickel]

This has bitten me as well. If you have any actions triggered by non-react-events then your calls to setState in those handlers are handled synchronously which may trigger the component to become unmounted halfway through your handler causing any subsequent calls to setState to emit a warning. This is very unintuitive and I would expect setState to always be asynchronous. The documentation even indicates that this is the likely scenario. The abstraction, as is, is very leaky because you need to know if your handler will invoke a call to setState somewhere in its call stack. For example,

componentDidMount() {
    window.addEventListener(...);
}

componentWillUnmount() {
    window.removeEventListener(...);
}

handleWindowKeyboardPress(event) { 
    this.props.onKeyPressed(); // triggers a datastore update which emits a change event that ends up invoking a setState call in a parent somewhere

    // component is now unmounted

    this.setState(...) // -> triggers warning
    // fix is this.isMounted && this.setState(...)
}

Checking this.isMounted should really not be necessary as now I must code in fear that my component will be unmounted at any point! This is exacerbated by the fact that this.isMounted has been deprecated thus there is almost no way to block these warnings aside from tracking my own isMounted manually!

[netpoetica]

For example,

componentDidMount() {
    window.addEventListener(...);
}

componentWillUnmount() {
    window.removeEventListener(...);
}

handleWindowKeyboardPress(event) { 
    this.props.onKeyPressed(); // triggers a datastore update which emits a change event that ends up invoking a setState call in a parent somewhere

    // component is now unmounted

    this.setState(...) // -> triggers warning
    // fix is this.isMounted && this.setState(...)
}

I am experiencing this exact some problem (attaching handlers in DidMount, removing in willUnmount, still getting a setState warning).

Are there any updates regarding this open issue? I even tried manually implementing an isMounted to verify, but I still end up with the same warning. Crazy thing is that the application still works fine, it just generates a ton of warnings.

[mmerickel]

I even tried manually implementing an isMounted to verify, but I still end up with the same warning.

Yeah you have to call it something like this._isMounted unfortunately or React will emit warnings about using the deprecated isMounted flag.

[netpoetica]

Haha thank you - no I mean the warnings are about setState :) I did call it something like bComponentMounted to avoid confusion.

[uriva]

+1 for setStateIfMounted for small harmless callbacks. Making the user install another library for promise cancellation or track the unmounting is tedious.

[rw3iss]

Seems I was able to get around the isMounted/setState inconsistencies by just declaring a separate 'private' variable 'mounted' on the instance, and setting it from componentWillMount() and componentWillUnmount(), ie:

class Test extends Component {
    constructor() {
        this.mounted = false;
    }
    componentWillMount() {
        this.mounted = true;
    }
    componentDidMount() {
        this.loadData();
    }
    componentWillUnmount() {
        this.mounted = false;
    }
    loadData() {
        var self = this;
        UserStore.getUser()
            .then((r) => {
                if (this.mounted) {
                    self.setState({ user: user });
                }
            });
    }
}

Seems to work as intended. If anyone knows why this might not be ideal, I'm all ears. Also would be nice if React could handle this implicitly. Seems ridiculous that it doesn't. Obviously we don't want to set a component's state if the component is no longer mounted, so don't let us (just fail silently). Sheesh.

[gaearon]

This discussion is a little old but I think we have a reasonable conclusion here.

If you can, we recommend that you use APIs that provide cancellation.

For example, axios lets you create a CancelToken for all requests and then cancel() them when the component unmounts:

componentDidMount() {
  this.cancelSource = axios.CancelToken.source();
  axios.get('/user/12345', {
    cancelToken: this.cancelSource.token
  }).then(
    // ...
  );
}

componentWillUnmount() {
   this.cancelSource.cancel()
}

I don’t agree cancellation is necessarily tedious—if it is, I suggest using a different library or asking the library you use to add a cancellation API.

This is the right way to solve this problem because it also eliminates a whole class of bugs.

For example, if you fetch data based on props, you should also cancel any requests for previous props after they change—or you risk getting the new response earlier than the older response, potentially displaying the wrong UI. Cancellation already solves these race conditions so this is one more reason to embrace it.

If you can’t (or choose not to) implement cancellation for some reason, then setting a field is the way to go.

Yes, this is clunky, but this clunkiness reminds you that:

• Your application is using network less efficiently.
• Your application might have more memory leaks.
• Your application might have more race conditions.

In any case, React doesn’t throw errors when this happens anymore. But we will intentionally keep the suboptimal case clunky so that the developers have incentives to implement the right solution (cancellation).

There are some edge cases related to libraries like Redux.

I would just recommend to not perform side effects in componentWillUnmount that might affect the tree being unmounted. This sounds like potential performance issue anyway. You can enqueue those side effects and flush them on the next tick, if you’d like.

So, to sum up:

• It is only a warning, not an error now.
• isMounted() is not available for ES6 classes anyway.
• The right fix is to implement cancellation, and libraries like axios make it easy.
• You can always either ignore the warning, or use a field.
• For cases like Redux, it’s better to dispatch on the next tick and let the tree unmount first.

I think this covers everything in this issue, so I’m closing it. Thanks to everyone for the discussion!

[gaearon]

Regarding the case in https://github.com/facebook/react/issues/2787#issuecomment-183426861, it seems like reordering setState call and this.props.onKeyPressed would solve the issue.

[idibidiart]

@gaearon

Can you contextualize this part of your comment for me, please?

"Your application is using network less efficiently"

Cancelling a Promisified HTTP request which ultimately calls xhr.abort simply unregisters the callback on the client side. It does not close the socket in the browser since many requests or tabs could be using it at once (AFAIK sockets are expensive so they are likely to be shared) Cancellation on client side will not automatically interrupt a db query on the server (moreover, if it happens to be a mutative operation it won't roll back automatically) so in all cases other than browser closing the socket (user closes browser) the server will end up sending the request to the client anyway, thus using network bandwidth.

I'd like to be corrected on this if I'm wrong on this or if there is more subtleness to your comment. Would be good to have greater context around this discussion that includes server and network.

Thanks,

[gaearon]

That's fair, I didn't know abort() doesn't close the socket! Thanks for correcting.

[zdila]

redux-logic has a nice cancellation mechanism.

[donaldpipowitch]

@gaearon Thank you very much for summarising the solutions to this issue. Exactly what I was looking for. Just one question to the existing "setting a field" solution. Is this the recommended way to do this (e.g. set the mounted flag in componentWillMount)? Just asking because the webpack docs have an example which handles this slightly differently (e.g. set the mounted flag as the first line in componentDidMount)? It looks strange to set a flag which says "this component is mounted" in a life cycle hook which says "this component will be mounted".

[gaearon]

I agree componentDidMount is a better place to set the flag to true.

[felixfbecker]

The native browser fetch API is promise based and cancellable through AbortController: https://developer.mozilla.org/en-US/docs/Web/API/AbortController/abort There are polyfills so every HTTP library can make use of AbortControllers.

RxJS Observable.ajax() is also cancellable.

[robwise]

I wrote a blog article demonstrating how to do use AbortController in a simple React use case. I'd appreciate any feedback if I got something wrong.

http://robwise.github.io/blog/cancel-whatwg-fetch-requests-in-react