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

useEffect memory leak when setting state in fetch promise #15006

Closed
ryansaam opened this issue Mar 3, 2019 · 31 comments
Closed

useEffect memory leak when setting state in fetch promise #15006

ryansaam opened this issue Mar 3, 2019 · 31 comments

Comments

@ryansaam
Copy link

ryansaam commented Mar 3, 2019

Do you want to request a feature or report a bug?
Reporting a possible bug

What is the current behavior?
My app renders fine with no errors but I can't seem to figure out why I keep getting this warning:

index.js:1446 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 a useEffect cleanup function.
in ArtistProfile (at App.js:51)
in component (created by Route)

api-calls.js (Here's a link):
https://github.com/ryansaam/litphum/blob/master/src/api-calls.js

App.js

class App extends Component {
  constructor(props) {
    super(props)
    this.state = {
      user: {},
      spotifyAPI: {}
    }
  }
  componentDidMount() {
    if (user_token) {
      sessionStorage.setItem('access_token', user_token)
      this.setState({
        spotifyAPI: new spotifyAPI( user_token )
      })
    } else if (sessionStorage.getItem('access_token')) {
      this.setState({
        spotifyAPI: new spotifyAPI( sessionStorage.getItem('access_token') )
      })
    }
  }
  componentDidUpdate(prevProps, prevState) {
    if (this.state.spotifyAPI !== prevState.spotifyAPI)
      this.state.spotifyAPI.getUserProfile()
      .then(data => this.setState({user: data}))
  }
  
  render() {
    const { user, spotifyAPI } = this.state
    const token = sessionStorage.getItem('access_token')
    return (
      <Router>
        <div className="App">
            { (spotifyAPI.user_token && user)
            ? (<div className="logged-in">
                <div style={{width: "250px", height: "100%", position: "relative", float: "left"}} >
                  <Nav image={user.images ? user.images[0].url : null} user={user} />
                </div>
                <main id="main">
                  <Route path={`/${user.type}/${user.id}`} exact component={() => <Home spotifyAPI={spotifyAPI} />} />
                  <Route path="/artist/" component={() => <ArtistProfile spotifyAPI={spotifyAPI} />} />
                </main>
              </div>) 
            : <div onClick={() => window.location = "http://localhost:8888/login"} >log in</div>
            }
        </div>
      </Router>
    );
  }
}

ArtistProfile.js

const  ArtistProfile = props => {
  const [artistData, setArtistData] = useState(null)
  const { getArtist, getArtistAlbums, getArtistTopTracks } = props.spotifyAPI

  useEffect(() => {
    const id = window.location.pathname.split("/").pop()
    const ac = new AbortController()
    console.log(id)
    Promise.all([
      getArtist(id, ac),
      getArtistAlbums(id, ["album"],"US", 10, 0, ac),
      getArtistTopTracks(id, "US", ac)
    ])
    .then(response => {
      setArtistData({
        artist: response[0],
        artistAlbums: response[1],
        artistTopTracks: response[2]
      })
    })
    .catch(ex => console.error(ex))
    return () => ac.abort()
  }, [])
  console.log(artistData)
  return (
    <div>
      <ArtistProfileContainer>
        <AlbumContainer>
          {artistData ? artistData.artistAlbums.items.map(album => {
            return (
              <AlbumTag
                image={album.images[0].url}
                name={album.name}
                artists={album.artists}
                key={album.id}
              />
            )
          })
          : null}
        </AlbumContainer>
      </ArtistProfileContainer>
    </div>
  )
}

What is the expected behavior?
If you can see in ArtistProfile.js I am using a clean up function that aborts when the component does unmount. The fetch would be aborted and state shouldn't update but for some reason I am still getting this memory leak warning.

What I am expecting is for the warning to no longer throw because am using a clean up function that aborts the fetch.

Link to repo: https://github.com/ryansaam/litphum

My stackoverflow question: https://stackoverflow.com/questions/54954385/react-useeffect-causing-cant-perform-a-react-state-update-on-an-unmounted-comp/54964237#54964237

Which versions of React, and which browser
React 16.8.2
Latest version of Chrome

@aweary
Copy link
Contributor

aweary commented Mar 4, 2019

What I am expecting is for the warning to no longer throw because am using a clean up function that aborts the fetch.

If you're getting that warning, it means that your setState call is still being executed. Are you sure your cancellation is working correctly? Have you verified that the cleanup function returned in useEffect is being called when you expect it to be? If it is, then React is working as expected and you'll need to investigate why your cancellation isn't working.

Also it looks like you're under-specifying dependencies for your useEffect hook. It uses methods from props.spotifyAPI, but it doesn't declare them in the dependency array. We have an ESLint plugin that will catch problems like this for you.

@ryansaam
Copy link
Author

ryansaam commented Mar 4, 2019

When loading the component I added a console.log() to the clean up function. The log gets called once when fetching data (not expected), and another when I click to a different page (what I expected). I don't know why it unmounts when loading data. But my clean up function should cancel requests.

@aweary
Copy link
Contributor

aweary commented Mar 4, 2019

Can you try and create a CodeSandbox that reproduces the problem? It's difficult to try and setup and run a full application that makes actual API calls and requires user tokens.

@ghengeveld
Copy link
Contributor

ghengeveld commented Mar 6, 2019

To avoid calling setState after unmount from a hook, you can use the useEffect callback to set a ref value on unmount, and use that as a guard in your promise callbacks to check whether the component is still mounted before updating state. Here's an example: https://github.com/ghengeveld/react-async/blob/master/src/useAsync.js

But to be honest you're probably better off not reinventing this wheel and use useAsync directly, because it's fully tested and covers other edge cases as well.

@ryansaam
Copy link
Author

ryansaam commented Mar 8, 2019

@ghengeveld Installing useAsync resolved the issue I was having

@ryansaam ryansaam closed this as completed Mar 8, 2019
@gaearon gaearon reopened this Mar 9, 2019
@gaearon
Copy link
Collaborator

gaearon commented Mar 9, 2019

Let's keep it open though. There might be a false positive warning in some cases.

It would really help if you turned this into a CodeSandbox...

@ryansaam
Copy link
Author

ryansaam commented Mar 9, 2019

Alright I will try my best to recreate it maybe using another API like a weather API. The only reason I couldn't here was because I was using Spotify and you need to have an account to get an access token

@ryansaam
Copy link
Author

I tried to recreate the warning but I'm not getting it to show in this demo
https://codesandbox.io/s/r5l297q3oq?fontsize=14

@carl0s-sa
Copy link

carl0s-sa commented Apr 22, 2019

Hi, I was getting this warning as well.
To reproduce you need to unmount the component calling useEffect before the promise resolves.
I've edited the (CodeSandbox) to add a 5 second delay.
Steps to reproduce:

  1. Click "See charmander"
  2. Right after button "Home" loads click it
  3. Wait for error to pop on console

Since we can't cancel out a Promise here's an article that explains a workaround.

I believe it would make a nice QOL improvement to supress the warning (or even prevent promise execution) if the component is unmounted.

Something like returning the Promise in useEffect and let internals take care of the "isMounted" validation?
Edit2: Was not aware that useAsync was a thing

Edit: in your example try putting a console.log before the promise resolving (or rejecting) and see if it's being called. In my project I also got random warnings like those until I "guarded" all of the state updates on promises. Never saw one again.

Edit2: Took a look at the codebase, you're not using the signal on the fetch calls so the ac.abort call is not aborting them. Like so

@xgqfrms
Copy link

xgqfrms commented Oct 17, 2019

same question

image

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 a useEffect cleanup function.

cancel promise solution

  const { adcode, regions = [] } = regionData;
  const [regionName, setRegionName] = useState('all');
  const [tableData, setTableData] = useState(regions);
  useEffect(() => {
    let isSubscribed = true;
    // getRegionName(adcode).then(setRegionName);
    getRegionName(adcode).then(() => {
      if(isSubscribed) {
        return setRegionName;
      }}
    );
    const promises = [];
    regions.forEach(item => {
      promises.push(
        getRegionName(item.code)
        .then(name => {item.name = name;})
        // eslint-disable-next-line no-console
        .catch(() => console.log('error', item.code)),
      );
    });
    Promise
    .all(promises)
    .then(() => {
      if(isSubscribed) {
        return setTableData(regions.filter(({ name }) => !!name));
      }
    });
    return () => isSubscribed = false;
  }, [adcode, regionData, regions]);

@giovanigenerali
Copy link

How to clean up subscriptions in react components using AbortController
https://medium.com/@selvaganesh93/how-to-clean-up-subscriptions-in-react-components-using-abortcontroller-72335f19b6f7

@lakhpatm
Copy link

Facing same issue

 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 a useEffect cleanup function.
    in ForwardRef (at PlanSelection.jsx:43)
const [planViewMinHight, setPlanViewMinHight] = useState(null);
  const carouselRef = useRef();

  useEffect(() => {
    if (planViewMinHight !== carouselRef?.current?.clientHeight) {
      setPlanViewMinHight(carouselRef?.current?.clientHeight);
    }
  }, [carouselRef, planViewMinHight]);

@malishahi
Copy link

I have the similar issue, I have canceled the axios request, but I get this Warning only on the first unmount of a component. This warning does not happen on the next umount of a component.

See below codes:

import React, { useEffect } from "react";
import CurrentOperatingStatus from "./current-operating-status"
import { useDataApi } from './lib/data-api'
import Controllers from './cos-table'

export default function ContentOverall() {
const [{ data, isLoading, isError, }, doFetch] = useDataApi(
'/rest/machine-list');

if (isError)
    return <div> Something went wrong...</div>;
else if (isLoading || data === null)
    return <div> Loading ...</div>;
else if (data && data.apiStatus.status == 'failure')
    throw new Error(response.data.apiStatus.message);

return (
    <div id="content" className="d-block col-auto FsUI_content">
        {/* content */}
        {/* content header */}
        <div className="w-100" id='1'>
            <div className="row FsUI_content_header">
                <div className="row d-xl-none w-100">
                    <ul className="nav FsUI_breadcrumbs">
                        <li>Overall Monitoring</li>
                    </ul>
                </div>
            </div>
        </div>
        <div className="row w-100 FsUI_block" id='2'>
            <CurrentOperatingStatus data={data?data.list:null} />
            <div className="col-auto FsUI_block">
                <div className="row align-items-end FsUI_block_header" id='1'>
                    <h6 className="FsUI_block_title">Current Operating Status List</h6>
                </div>
                <div className="FsUI_block_base" id='2'>
                    <Controllers data={data?data.list:null} />
                </div>
            </div>
        </div>
    </div>);

}
----data-api.js----
import { useState, useEffect } from 'react';
import axios from "./axios";

const useDataApi = (initialUrl) => {
const [data, setData] = useState(null);
const [url, setUrl] = useState(initialUrl);
const [isLoading, setIsLoading] = useState(false);
const [isError, setIsError] = useState(false);
const CancelToken = axios.CancelToken;
const source = CancelToken.source();

useEffect(() => {
    const fetchData = async () => {
        setIsError(false);
        setIsLoading(true);
        try {
            const result = await axios.get(url, {
                cancelToken: source.token
            });
            setData(result.data);
        } catch (error) {
            if (axios.isCancel(error)) {
                console.log('Request canceled', error.message);
            } else {
                // handle error
                setIsError(true);
            }
        }
        setIsLoading(false);
    };
    fetchData();

    return () => {
        source.cancel('Canceling HTTP request in useEffect cleanup function...');
    }
}, [url]);

return [{ data, isLoading, isError, source }, setUrl];

};

export { useDataApi };

@danisanc
Copy link

I have a similar problem

I open a websocket connection and store it in a useState, I pass that connection into props for 4 routes.

Every time I launch an "emit" on these pages, I get an error like that for each route I visited before

image

This is the code of my page

import React, { useState, useEffect } from "react";
import { useCookies } from "react-cookie";

import api from "../../services/api";
import apiHeader from "../../utils/apiHeader";

import ModalCaller from "../../components/modalCaller";

const WaitingPage = ({ websocket }) => {
  const [menuOpen, setMenuOpen] = useState(false);
  const [passwords, setPasswords] = useState([]);
  const [pass, setPass] = useState(false);
  const [cookies] = useCookies(["token"]);

  const handleToggleModalCaller = (toggle, currentPass) => {
    websocket.emit("call", { pass: currentPass });

    setMenuOpen(toggle);
    setPass(currentPass);
  };

  // Eventos websocket
  useEffect(() => {
    if (websocket) {
      websocket.on("call", pass => {
        setPasswords(passwords.filter(value => value.id !== pass.id));
      });
    }
  });

  // Carrega senhas na fila
  useEffect(() => {
    api.get("/passwords?status=1", apiHeader(cookies.token)).then(res => {
      setPasswords(res.data);
    });
  }, [cookies.token]);

  return (
    <main className="adminPageBody">
      {menuOpen && <ModalCaller setState={setMenuOpen} pass={pass} />}

      <h3>Aguardando ({passwords.length})</h3>

      {passwords.map((pass, i) => (
        <div className="card" key={i}>
          <div>
            <h4>{pass.fullPassword}</h4>
            <p>{pass.services.name}</p>
            <p>{pass.dob}</p>
          </div>

          <button onClick={() => handleToggleModalCaller(true, pass)}>
            Chamar
          </button>
        </div>
      ))}
    </main>
  );
};

export default WaitingPage;

@TomPradat
Copy link

TomPradat commented Mar 13, 2020

Is is so bad to leave this warning ? Does it really impact performances ?

I've been wondering about the case where you would want to dispatch actions anyways (for example after a PUT/POST request where you want to update your context even if the component umounts).

Then you would have to handle a "cancel logic" in your context (in case your context is unmounted).

I tried it in the "Fetch and store in a context" example here.

This looks painful though, shouldn't we be able to tell react to ignore the call and not have a warning ?

@malishahi
Copy link

The solution to this problems it to use a flag to check if a component is unmounted, avoid setting state update, as the following in a useEffect hook.

        let ignore = false;
.
.
.
        if (!ignore)
            setServerError(errors);

        return () => {
            ignore = true;
        }

@TomPradat
Copy link

The solution to this problems it to use a flag to check if a component is unmounted, avoid setting state update, as the following in a useEffect hook.

        let ignore = false;
.
.
.
        if (!ignore)
            setServerError(errors);

        return () => {
            ignore = true;
        }

Yes but what if you're dispatching actions to a context and want to dispatch even if the component unmounts ? For exemple you have a popin with a form that change the color of your website. You would still want to update the website color even if the user has closed the popin while the API call was still being processed

@malishahi
Copy link

How about exposing (exporing) a setColor from a top level component that is always mounted, and use this setColor inside popin component?

@TomPradat
Copy link

Well yeah this would work but that's not the point, you could have a context that is not always mounted. The question being "What happens when react prints this warning", is it harmful in some way ? If so how should we handle aisMounted boolean in the contexts ?

@denissanthiago
Copy link

denissanthiago commented May 12, 2020

@TomPradat
Copy link

Well yeah this would work but that's not the point, you could have a context that is not always mounted. The question being "What happens when react prints this warning", is it harmful in some way ? If so how should we handle aisMounted boolean in the contexts ?

@gaearon @bvaughn Any thoughs on this ?

@adiled
Copy link

adiled commented May 13, 2020

@TomPradat What's harmful is the lack of one's thought alignment with useEffect. The docs haven't been updated since adaptation phase, so they still say it's a cool replacement of lifecycle methods. It's tricky understanding what's under the hood just like flux was when it first came out, but you should get right to it instead of circling around the warning.

@TomPradat
Copy link

I'm not circling around the warning, I'm trying to understand if there is an idiomatic way to handle this that i would have missed.

I don't want to overly complicate my apps for edge cases that will rarely happen. Though I'm interested what ppl think about those edge cases. What's more I'm not sure that "wanting to update a context after a POST request" is such a rare use case.

I'll take a look at the useLayoutEffect hook 👍

@bfdes
Copy link

bfdes commented May 27, 2020

I get the impression that an isMounted flag will mitigate the race condition in most cases. But there could still be a problem if the component unmounts after the callback guard is evaluated, but just before a state update is scheduled with the fetch payload. It doesn't look like this can be fixed by using the lifecycle methods of class components either.

In practice, this could only be an issue if an impatient user navigates away just as the data comes in.

@ucheuzor
Copy link

ucheuzor commented Aug 6, 2020

I dont understand the solutions given here. Am also facing same issue

@cargallo
Copy link

return () => {
            ignore = true;
        }

It is obvious that if this simple trick makes the difference it's because React is bad designed. They cared so much about the management of the state that the state ended up managing them.

@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Dec 25, 2020
@kaveiros
Copy link

kaveiros commented Apr 13, 2021

Just got the same warning today. I will try to implement the solution proposed here.

My code is:
`import React, {Component, useEffect, useState} from 'react'
import {
Col, Row, Grid, Panel, Container, ButtonToolbar, Breadcrumb,
Button, Icon, Content, ControlLabel, FormControl, HelpBlock, FormGroup, Form, Header, Steps
} from 'rsuite'
import LoaderHook from "../common/LoaderHook";
import {v4 as uuidv4} from "uuid";
import WarehouseStep1 from "./WarehouseStep1";
import WarehouseStep2 from "./WarehouseStep2";
import {useLocation} from "react-router-dom";
import WarehouseService from "../../services/WarehouseService";
import {showSaveErrorNotification, showSuccessNotification} from "../common/Notifications";
import SectorService from "../../services/SectorService";

const WarehouseTab = () => {

const url = '/warehouse'
const successString = 'Το υλικό αποθηκεύτικε'
const errorString = 'Σφάλμα στην αποθήκευση του υλικού'
const warehouseService = new WarehouseService(url)
const [step, setStep] = React.useState(0);
const uniqueVersion = useState(uuidv4())
const [loading, setIsLoading] = useState(false)
const initState = {
    aa: 0,
    code:'',
    kind:'',
    description:'',
    quantity:'',
    section:'',
    sector:'',
    nameOfPersonAccepted:'',
    uniqueVersion: uniqueVersion[0],
    additionalInfo:[]
}

const initErrorState = {
    aa: 0,
    code:'',
    kind:'',
    description:'',
    quantity:'',
    section:'',
    sector:'',
    nameOfPersonAccepted:''
}

const [errors, setErrors] = useState(initErrorState)
const [hasValidationError, setValidationError] = useState(true)
const location = useLocation()
const [warehouseState, setWarehouseState] = useState(initState)
const [sectors, setSectors] = useState()
const [sections, setSections] = useState()

//If loaded from edit button, then populate fields
useEffect(() => {
    if(location.state !== undefined) {
        setWarehouseState(location.state)
    }
},[location])

//fetch sectors
useEffect(() => {
    SectorService.getAllSectors().then(response => {
        setIsLoading(true)
            let sectors = []
            for (let sec of response.data){
                let secData = {
                    "label": sec.sector,
                    "value": sec.sector
                }
                sectors.push(secData)
            }
            setSectors(sectors)
            setIsLoading(false)
    })
        .then(err => {
            console.log(err)
            setIsLoading(false)
        })
},[])

//fetch sections
useEffect(()=> {

})


const handleChange = (name) => (event) => {
    //TO DO check for validation errors later
    setWarehouseState({...warehouseState, [name]: event})
}

const handleSubmit = () => {
    //TO DO check for errors later
    setIsLoading(true)
    warehouseService.save(initState).
        then(response => {
            setIsLoading(false)
        showSuccessNotification(successString)
    }).catch( err => {
        setIsLoading(false)
        setWarehouseState(initState)
        setErrors(initErrorState)
        showSaveErrorNotification(errorString)
    })


}

const handleStep = (stepValue) => (evn) => {
    setStep(stepValue)
}

return (
    <Panel>
        <Header>
            <Breadcrumb>
                <Breadcrumb.Item href="/">Αρχική</Breadcrumb.Item>
                <Breadcrumb.Item href="/warehouse" active>Αποθήκη</Breadcrumb.Item>
            </Breadcrumb>
        </Header>
        <Content>
            <Panel shaded bordered>
                <Steps current={step}>
                    <Steps.Item title="Βασικά στοιχεία" />
                    <Steps.Item title="Παρατηρήσεις" />
                </Steps>
            </Panel>
            <hr />
            <Panel shaded bordered>
                {loading&&<LoaderHook message={"Γίνεται επεξεργασία..."}/>}
                {step === 0 ? <WarehouseStep1 {...warehouseState} errors={errors} hasValidationError={hasValidationError}
                                             handleChange={handleChange} handleStep={handleStep} /> :
                    <WarehouseStep2 {...warehouseState} errors={errors} handleChange={handleChange}
                                   handleStep={handleStep} handleSubmit={handleSubmit} uniqueVersion={uniqueVersion[0]} />}
            </Panel>
        </Content>
    </Panel>
)

}

export default WarehouseTab
`

and the message:
index.js:1 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 a useEffect cleanup function. in WarehouseTab (at AuthenticatedRoute.js:13) in Route (at AuthenticatedRoute.js:6) in AuthenticatedRoute (at App.js:39)

Using Axios to setState

@murffious
Copy link

murffious commented Sep 22, 2021

Super common problem to call an api async and want to update state with use effect .... not sure how this is still misunderstood.. that seems like every single app would use that.

There are TONS of blog posts about about how to solve for this. Here is one I thought was well done: https://www.debuggr.io/react-update-unmounted-component/
Another one with similar thoughts kinda harder to read but some good ideas in my view -https://www.py4u.net/discuss/291052

SO has literally dozens or who knows maybe even more questions. Here is a recent example:
https://stackoverflow.com/questions/67541403/cant-perform-a-react-state-update-on-an-unmounted-component-warning-in-reactjs

I did some digging into this. As mentioned/demonstrated, this is a VERY common problem. The logic shown above links for solving this can be encapsulated nicely into a hook / npm module. See here for one example (there are many with all very similar names) :https://github.com/helderburato/use-is-mounted-ref/blob/main/src/use-is-mounted-ref.ts

One would naturally lean on the amazing react team and their docs for clarity on best practice. But that could lead to confusion for this issue in my view see here reactjs/react.dev#1082. Not sure if this is directly related or not #22114 but I can't seem to find this warning we are all talking about in the react source code to inspect further. So maybe they took it out.

In fact my team had been using 3 different variations of the aforementioned solutions which I found while tasked to refactor parts of for various reasons. I was still seeing the warning in the console even after applying strategies aforementioned. Then I tried this one which is same as the others yet goes even further to add a useCallback in there at the end. It seems to be working the best: https://github.com/hupe1980/react-is-mounted-hook/blob/master/src/use-is-mounted.tsx
implemented as --


export default function useIsMounted(): () => boolean {
  const ref = useRef(false);

  useEffect(() => {
    ref.current = true;
    return () => {
      ref.current = false;
    };
  }, []);

  return useCallback(() => ref.current, [ref]);
}```


@murffious
Copy link

Given that the latest release was 17.0.2 (March 22, 2021) and this warning was removed on Aug 18 #22114 -- this issue is not going to be an issue someday based on that eh

@gaearon
Copy link
Collaborator

gaearon commented Mar 29, 2022

React 18 does not have this warning because it's often pointless. (Such as in this case.)

@gaearon gaearon closed this as completed Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests