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

Environment's held on longer than they need too #3013

Closed
maraisr opened this issue Feb 6, 2020 · 0 comments
Closed

Environment's held on longer than they need too #3013

maraisr opened this issue Feb 6, 2020 · 0 comments

Comments

@maraisr
Copy link
Contributor

maraisr commented Feb 6, 2020

When using useFragment instead of createFragmentContainer, in an SSR context the resourceMap holds onto the environment longer than it needs too, which causes the application to look like its got a memory leak, as the store is contained inside the environment. As touched on #2854 (comment)

I will prefix that with being, that in an SSR setting, you'd ideally want to blow away the store on every request, so you're not leaking store data between requests. So this only came about, when you create a new Environment, for the request. And having the resourceMap Map hold onto now discarded environments.

const dataResources: Map<IEnvironment, FragmentResource> = new Map();
function getFragmentResourceForEnvironment(
environment: IEnvironment,
): FragmentResourceImpl {
const cached = dataResources.get(environment);
if (cached) {
return cached;
}
const newDataResource = createFragmentResource(environment);
dataResources.set(environment, newDataResource);
return newDataResource;
}

My suggestion is to move away from a Map, and into a WeakMap, so that once the environment gets gc'd, it cleans up the fragment resourceCache.

- const dataResources: Map<IEnvironment, FragmentResource> = new Map();
+ const dataResources: WeakMap<IEnvironment, FragmentResource> = new WeakMap(); 
@maraisr maraisr changed the title Environment's held on longer than they need to Environment's held on longer than they need too Feb 6, 2020
jstejada pushed a commit that referenced this issue Feb 13, 2020
Summary:
With this PR I aim to address the situation where environments are removed, but never garbage collected as references are held onto in this Map.

A simple case could be:

```js
const MyApp = () => {

	const [environment, setEnvironment] = useState(new Environment({ network, store }));

	useEffect(() => {
		setTimeout(() => {
			setEnvironment(new Environment({ network, store }));
		}, 1e3);
	}, []);

	return (<RelayEnvironmentProvider environment={environment}>
		<MyComponentTreeWith_A_useFragment/>
	</RelayEnvironmentProvider>);
};
```

as you'll find from that example, the `dataResources` map will be forever growing, with nothing clearing.

fixes: #3013
Pull Request resolved: #3014

Reviewed By: jstejada

Differential Revision: D19797050

Pulled By: alunyov

fbshipit-source-id: 4ef18e7634c7d50597944967e9ffc0a330ac3633
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant