feat: add ignore method to IncomingMessage #35767
Conversation
That method makes possible to indicate that reponse is ignored and all data chunks should be dropped.
|
@kyrylo-hrechykhin Thanks for the PR! Since this is a new method, would it be possible to add a test or two for it, just to make sure we can catch it in the future if it regresses? |
There was a problem hiding this comment.
if my understanding is correct, the changes in #25531 made it such that memory leaks now happen more easily/by default unless the user utilizes the API added in this PR?
If that's the case, I'd really like to try to see if there's an alternate solution that allows for us to fix this problem without creating a new, potentially unintuitive API which users must use to prevent the leaks. It's not that i'm against the API design per se, but rather I think that adding new API to address what is arguably a bug is probably not the most effective solution.
| Indicates that response is ignored. Cancels all chunks of data from being downloaded. | ||
|
|
||
| **Note:** Please make sure you call this method if response data is not read. | ||
| Otherwise it will lead to memory leaks in browser process ([#34158](https://github.com/electron/electron/issues/34158)). |
There was a problem hiding this comment.
We don't want to link to bugs in our documentation I wouldn't say.
Thanks for the comment. Absolutely agree that it is better to fix it properly rather than modify API in order to avoid memory leaks. I did not find solution that will be better in this case when I created this PR. I will check if it is possible to pin body chunk getter on first resume callback call. It will indicate that response is being read. |
|
Could we use a FinalizationRegistry to watch for GC? Or hook the destructor on the C++ side? |
|
@kyrylo-hrechykhin would you still like to see this move forward? |
|
I'm closing this PR due to staleness. Please feel free to reopen if there's renewed interest in continuing this work! 🙇 |
Description of Change
That method makes possible to indicate that response is ignored, and all data chunks should be dropped. Otherwise, response data should be read. If response data is not read and is not ignored, memory leak will happen per each net.request call in browser process.
Fixes #34158
CC: @VerteDinde
Checklist
npm testpassesRelease Notes
Notes: Method ignore() is added to IncomingMessage in order to prevent memory leaks in browser process when data is not read.