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

Memory leak issues when upgrading form parse server ‘~2.x’ to ‘~3.x’ #6061

Closed
n-so opened this issue Sep 18, 2019 · 9 comments · Fixed by #6126

Comments

@n-so
Copy link

commented Sep 18, 2019

Issue Description

Hi,
When upgrading from parse-server ‘~2.x’ to ‘~3.x’
I started getting memory leaks issues.
I searched the web and tried many ways to fix this, the only solution that worked was to set enableSingleSchemaCache: true,
But I do not what to use this solution for production .(I don't want to take any risks)
Is there any other solution for this?

attached is a screenshot : comparison of two heap dumps taken 2 minutes apart
image

Environment Setup

  • Server

    • parse-server version (Be specific! Don't say 'latest'.) :'~3.x'
    • Operating System: [FILL THIS OUT]
    • Hardware: [FILL THIS OUT]
    • Localhost or remote server? AWS
  • Database

    • MongoDB version: 4.0.3
    • Storage engine: [FILL THIS OUT]
    • Hardware: [FILL THIS OUT]
    • Localhost or remote server? AWS

Logs/Trace

No errors or strange messages reported, everything working as usual, just memory increasing.

@dplewis

This comment has been minimized.

Copy link
Member

commented Sep 18, 2019

@n-so You can check out this for possible solutions when not using enableSingleSchemaCache. I don't believe those solutions have been implemented since a solution already exists. Feel free to try and test them for yourself. We will try to look into it also.

@acinader @davimacedo Should enableSingleSchemaCache be set to true by default? I think it pretty harmless.

@davimacedo

This comment has been minimized.

Copy link
Member

commented Sep 19, 2019

Yes. I think it should be set by default. We'd need to mention it in the next release and ideally also the downsides of it.

@n-so

This comment has been minimized.

Copy link
Author

commented Sep 19, 2019

Hi @davimacedo ,
Are there any risks when setting enableSingleSchemaCache to true?
What are the downsides of enableSingleSchemaCache?

@dplewis

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

The current implementation creates a new cache per request. This is to prevent side effects between requests. Using enableSingleSchemaCache is basically the same as disabling cache.clear() while using the same cache for all requests.

Does cache.clear() gets called after every request? If not, implementing that would fix your memory issue while minimizing side effects

@dplewis

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

@davimacedo does cache.clear() gets called. I couldn’t find it. Where should we put it? RestQuery / RestWrite execute?

@davimacedo

This comment has been minimized.

Copy link
Member

commented Sep 20, 2019

@dplewis @n-so Just to make sure that we are all in the same page. This is what happens at each request:

  • The middleware (which run each request) retrieves the Config object here
  • In order to return the Config object, actually, at each request, a new Config instance is created and a new instance of SchemaCache as well here
  • Here you can find the difference between enableSingleSchemaCache=true and enableSingleSchemaCache = false

If enableSingleSchemaCache is enabled, a single key is used and therefore the same cache is re-used in all requests. On the other hand, if enableSingleSchemaCache is disabled, a new key is generated at each request. The memory leak happens because the cache that was stored in each request is never cleaned. So, at each request, more cache is accumulated unnecessarily, and the memory continues growing.

I was first thinking about adding a middleware here to clean the cache at the end of each request. But I am afraid of jobs, push notifications (others?) routines that could have to continue running even after returning the response to the client.

Another option would be a TTL (maybe an additional parameter to Parse Server) associated to each of these keys. Then we could have a process cleaning these caches.

@acinader @dplewis thoughts?

@kcolton

This comment has been minimized.

Copy link

commented Sep 25, 2019

I was first thinking about adding a middleware here to clean the cache at the end of each request. But I am afraid of jobs, push notifications (others?) routines that could have to continue running even after returning the response to the client.

Worst case If something referenced the cache after it was cleared at end of requests, wouldn't it just refetch the schema from the DB? same as if the cache happened to expire.

Another option would be a TTL (maybe an additional parameter to Parse Server) associated to each of these keys. Then we could have a process cleaning these caches.

This is different than schemaCacheTLL?

Seems like schemaCacheTLL should already handle freeing the space associated with the schema cache keys themself. Are you referring to "keys" on the Config object generated per request? Not sure I follow there.

Guess I'm not following how schema values in the various schema caches would be source of memory leak when there is a schemaCacheTTL at work. Is it from the nature of how lru-cache is implemented?

Edit: Looked at lru-cache and yeah, now that I see that there is nothing proactively cleaning keys older than maxAge. I'm following the logic now. Makes sense to me to try to clean that up explicitly then, assuming that the only side effect is potentially needing to refetch schema from db in some special cases.
https://www.npmjs.com/package/lru-cache#options

Does this mean that user/session cache will leak memory as well from same mechansism?

We just switched one of our production servers to use in memory cache instead of redis (w/ enableSingleSchemaCache: true) and noticing memory usage slowly increasing when compared w/ the instances using redis cache still.

@dplewis

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

@kcolton I agree, I don’t think clearing the cache after each request would have side effects.

The SchemaCache is used to checking if classes and fields exist in the database without hitting the DB. Pushes and Jobs have already made writes and queries. After the request is over I don’t see how they would still use the cache.

I can do a PR and see how this would run against tests.

@davimacedo

This comment has been minimized.

Copy link
Member

commented Sep 27, 2019

If you take a look here, you will see that the job is started and left executing while the response is returned right away. It means that the request will end (and the cache will be cleaned) before the job finishes to execute. The push notification has a similar process. Probably other features as well. I believe that cleaning the session will not generate bugs as the schema will probably be cached again in the next time that the job needs it. But processes like jobs and push notifications will continue being cause of memory leak. That's why I think that a TTL solution would be the best one. But cleaning the cache in the end of each request will probably solve 99% of the problem. And since it seems really easy to be implemented, we should probably go for it. Will you tackle this one, @dplewis ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.