{"action":"created","issue":{"url":"https://api.github.com/repos/facebook/relay/issues/754","repository_url":"https://api.github.com/repos/facebook/relay","labels_url":"https://api.github.com/repos/facebook/relay/issues/754/labels{/name}","comments_url":"https://api.github.com/repos/facebook/relay/issues/754/comments","events_url":"https://api.github.com/repos/facebook/relay/issues/754/events","html_url":"https://github.com/facebook/relay/issues/754","id":127571449,"number":754,"title":"queryCache module-wide variables leaking memory on the server side","user":{"login":"fabiosantoscode","id":1611595,"avatar_url":"https://avatars.githubusercontent.com/u/1611595?v=3","gravatar_id":"","url":"https://api.github.com/users/fabiosantoscode","html_url":"https://github.com/fabiosantoscode","followers_url":"https://api.github.com/users/fabiosantoscode/followers","following_url":"https://api.github.com/users/fabiosantoscode/following{/other_user}","gists_url":"https://api.github.com/users/fabiosantoscode/gists{/gist_id}","starred_url":"https://api.github.com/users/fabiosantoscode/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/fabiosantoscode/subscriptions","organizations_url":"https://api.github.com/users/fabiosantoscode/orgs","repos_url":"https://api.github.com/users/fabiosantoscode/repos","events_url":"https://api.github.com/users/fabiosantoscode/events{/privacy}","received_events_url":"https://api.github.com/users/fabiosantoscode/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":17,"created_at":"2016-01-20T00:12:31Z","updated_at":"2016-07-25T20:24:20Z","closed_at":null,"body":"Using isomorphic-relay to get relay to work on the server, and using a load testing tool to create repeated requests and find a memory leak in an application I'm building, I observed that 2 maps called queryCache were growing unbounded.\r\n\r\nThese maps are in getRelayQueries.js, and in buildRQL.js, and they grow to thousands of keys over 600 requests to a page I have with no less than 2 containers.\r\n\r\nLonger explanation in:\r\n\r\nhttps://github.com/facebook/relay/pull/683#issuecomment-173021689\r\n\r\nI'll do my best to provide an example if it's necessary to reproduce the issue."},"comment":{"url":"https://api.github.com/repos/facebook/relay/issues/comments/235073191","html_url":"https://github.com/facebook/relay/issues/754#issuecomment-235073191","issue_url":"https://api.github.com/repos/facebook/relay/issues/754","id":235073191,"user":{"login":"KCraw","id":8075821,"avatar_url":"https://avatars.githubusercontent.com/u/8075821?v=3","gravatar_id":"","url":"https://api.github.com/users/KCraw","html_url":"https://github.com/KCraw","followers_url":"https://api.github.com/users/KCraw/followers","following_url":"https://api.github.com/users/KCraw/following{/other_user}","gists_url":"https://api.github.com/users/KCraw/gists{/gist_id}","starred_url":"https://api.github.com/users/KCraw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/KCraw/subscriptions","organizations_url":"https://api.github.com/users/KCraw/orgs","repos_url":"https://api.github.com/users/KCraw/repos","events_url":"https://api.github.com/users/KCraw/events{/privacy}","received_events_url":"https://api.github.com/users/KCraw/received_events","type":"User","site_admin":false},"created_at":"2016-07-25T20:24:20Z","updated_at":"2016-07-25T20:24:20Z","body":"Sorry to drag this convo out here, but I think it's relevant to anyone dealing with query aggregation and how to properly implement it with Relay, and not just iso-relay, react-relay-router, etc. \r\n\r\n> I don't think this solves the problem, since the constructor would presumably be shared amongst all QueryAggregator instances. This would mean that all aggregators would use the fragments of whatever aggregator happened to have been seen first - not good.\r\n\r\nThis brings up a concern with aggregation generally. `QueryAggregator` (as implemented in react-router-relay) appears to be generating responses to requests for fragments and queries dynamically, so when `getRelayQueries` is building its cache, we end up with with a single Component key in the `queryCache` which maps to a cache object with (eventually) a key for every possible route combination; e.g., something like this on the client (generated without SSR):\r\n\r\n<img width=\"1371\" alt=\"screen shot 2016-07-25 at 1 36 30 pm\" src=\"https://cloud.githubusercontent.com/assets/8075821/17112720/dec998a4-526c-11e6-9494-6de91204193b.png\">\r\n\r\nFrom your perspective, is this problematic with how Relay does things internally?\r\n\r\n> It isn't clear why a new aggregator would need to be recreated when re-encountering a previously seen route, as only the params on the Route object change.\r\n\r\nMy concern is whether anything async is happening between the time one request initiates use of the `QueryAggregator` and when it is released following the SSR. If so, another req for the same path from another user could modify the `QueryAggregator` while it is in use. If Relay doesn't do anything async until the actual GraphQL request, we could use one `QueryAggregator` cache for preparing data and another for the actual SSR, but you'd still have to guarantee nothing async would ever happen between checking out the `QueryAggregator` and the GraphQL request , and that's a problem if you use promises anywhere. You'd need to keep some sort of pool of QueryAggregators, you could even do it per unique route, but that would only slow down the memory leak.\r\n\r\nOr as you suggest, just enable turning off the cache on the server. Definitely the simplest and solution. Perfectly happy to work on it and submit a PR.\r\n\r\nHowever, based on your reply, I'd like to verify that there isn't a problem with how `QueryAggregator` interacts with the query cache on the client, where we can't just turn the cache off to get around the issue. If so, then a completely different solution for query aggregation might be needed. \r\n"}}
{"actor":{"display_login":"KCraw"}}