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

Potential memory leak in services/api/index.ts #21

Open
jasnell opened this issue Aug 4, 2020 · 6 comments
Open

Potential memory leak in services/api/index.ts #21

jasnell opened this issue Aug 4, 2020 · 6 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@jasnell
Copy link
Contributor

jasnell commented Aug 4, 2020

In the connected function in https://github.com/covidgreen/covid-green-app/blob/current/services/api/index.ts#L47-L60 https://github.com/covidgreen/covid-green-app/blob/current/src/services/api/index.ts#L46-L59, when retry is true and the network is not available, the function recursively awaits on itself. This sets up a memory leak when the network is unavailable for a protracted period of time (memory leak in that the Promise chain will grow indefinitely while waiting for the network to become available.

The code in question is copied below:

const connected = async (retry = false): Promise<boolean> => {
  const networkState = await NetInfo.fetch();
  if (networkState.isInternetReachable && networkState.isConnected) {
    return true;
  }

  if (retry) {
    throw new Error('Network Unavailable');
  } else {
    await new Promise((r) => setTimeout(r, 1000));
    await connected(true);
    return true;
  }
};

This should be refactored to avoid the recursion.

@jasnell jasnell added bug Something isn't working good first issue Good for newcomers labels Aug 4, 2020
@amis-shokoohi
Copy link

I don't know if I'm missing something but
when retry is true and networkState checks are falsy, the function throws an error but
when it's false, the function is gonna call itself with true arg, which again ends up throwing an error if network is unavailable.

@amis-shokoohi
Copy link

Uh everything's changed 😳
the link to index returns 404

@jasnell
Copy link
Contributor Author

jasnell commented Sep 9, 2020

Updated the link. The source was restructured into the src directory.

@jasnell
Copy link
Contributor Author

jasnell commented Sep 9, 2020

And yes, I saw the retry gating there... still better to avoid the recursion, as it's simply not necessary.

const checkNetworkState = async () : Promise<boolean> => {
  const networkState = await NetInfo.fetch();
  return networkState.isInternetReachable && networkState.isConnected;
};

const connected = async () : Promise<boolean> => {
  if (!(await checkNetworkState())) {
    await new Promise(r => setTimeout(r, 1000));
    if (!(await checkNetworkState()))
      throw new Error('Network unavailable');
  }
  return true;
};

@amis-shokoohi
Copy link

Thank you
This implementation is much better.
May I send your code as a PR?

@jasnell
Copy link
Contributor Author

jasnell commented Sep 9, 2020

Certainly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants