Fix memory leak issue when using fetch in oidc authentication #2640
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Great find!! I'm curious, do you know if this is reproducible in other environments and not just linux? |
| @@ -66,6 +66,12 @@ export default class BaseAuthenticator extends SimpleAuthBaseAuthenticator { | |||
| headers: { Authorization: `Bearer ${token}` }, | |||
| }), | |||
| ); | |||
|
|
|||
| // Note: Always consume response object in order to avoid memory leaks. | |||
| // visit https://undici.nodejs.org/#/?id=garbage-collection for more info. | |||
There was a problem hiding this comment.
Just want to point out we don't use undici since this is a node package, but is still relevant since it points out the fetch spec depends on garbage collection which means this behavior might not be well defined when depending on garbage collection. Might be worth clarifying that in this comment?
Also, should we consider making a HEAD request instead as suggested in your link and here if we don't need the body? I don't think there's much data returned so it might not matter too much either way
There was a problem hiding this comment.
thank you for the feedback!! comment updated!
i tested out using the head method tho i get the "method not allowed" error in the console :(
There was a problem hiding this comment.
Ah I guess we don't support HEAD requests 😢, I wonder how much actual work that is from the backend but either way I think it's fine if we just keep making GET requests
Description
When making a
fetchrequest that returns a response body, we have to consume the body in order to avoid undefined behavior. If we do not consume response body then its up to the env. garbage collection to free up the memory. In linux this was causing a memory leak during pending oidc authentications. Therefore, it is best practice to always consume response body (with.json). This github issue was illustrates what we were also running into.This memory leak issue for oidc authentication is visible in linux desktop client v2.2.0. But was not able to reproduce in mac. (haven't tried windows)
Before desktop client v2.2.0 this was not an issue because we were using
ember-fetchbut was introduced in this commit since we migrated away from that package and started usingfetchnative to node.Screenshots (if appropriate)
Before:
Screen.Recording.clip.2.mov
After:
Screen.Recording.clip.mov
How to Test
Checklist