-
Notifications
You must be signed in to change notification settings - Fork 566
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
use FinalizationRegistry for cloned response body #3458
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh. Simple solution.
Impressive.
LGTM
Wouldnt it be maybe better to do this in cloneResponse? undici/lib/web/fetch/response.js Line 330 in 6f912bf
Would this line have the same issue, if we dont put the response into the FinalizationRegistry? Line 323 in 6f912bf
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I went for the simplest and least invasive solution, but that does leave other holes left to fill. I'll fix it up. |
@KhafraDev seems this conflicts now, can you resolve them? |
Signed-off-by: Matteo Collina <hello@matteocollina.com>
running test with
node --expose-gc --test test/fetch/fire-and-forget.js
Before
After