-
-
Notifications
You must be signed in to change notification settings - Fork 935
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
Fix memory leak bug when re-using abortSignal
(#2160)
#2161
Conversation
@@ -545,6 +544,10 @@ export default class Request extends Duplex implements RequestEvents<Request> { | |||
return this; | |||
} | |||
|
|||
private abortHandler(): void { |
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.
nit
private abortHandler(): void { | |
private _abortHandler(): void { |
@@ -664,6 +667,7 @@ export default class Request extends Duplex implements RequestEvents<Request> { | |||
response.once('end', () => { | |||
this._responseSize = this._downloadedSize; | |||
this.emit('downloadProgress', this.downloadProgress); | |||
this.removeListener('abort', this.abortHandler); |
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.
This should be called on the signal instead
@@ -674,6 +678,7 @@ export default class Request extends Duplex implements RequestEvents<Request> { | |||
response.destroy(); | |||
|
|||
this._beforeError(new ReadError(error, this)); | |||
this.removeListener('abort', this.abortHandler); |
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.
response
is not guaranteed, but request
is. However listening on error
request event is also incorrect because of retries. I'd suggest to move this to _destroy
.
There is a possible fix here if it helps: marcelmeulemans@1bc4982 |
Great! I think this implementation fix #2160 correctly. I will close this PR due to wrong implementation, and there comes out possible fix now. |
Would you like me to open a PR for that fix? If so any ideas for how to write a unit test for the issue? |
About unit test, I think you better ask for the question to this repository's maintainers. |
FYI #2162 I considered using the |
Checklist
This PR trying to fix #2160. (WIP)
abortHandler
of request instance will be removed when the response end or error, abort.