Skip to content

fix: do not force-set req.destroyed = true on abort#1774

Merged
titanism merged 1 commit into
forwardemail:masterfrom
afharo:remove-v14-destroy-before-abort-hack
Aug 15, 2023
Merged

fix: do not force-set req.destroyed = true on abort#1774
titanism merged 1 commit into
forwardemail:masterfrom
afharo:remove-v14-destroy-before-abort-hack

Conversation

@afharo

@afharo afharo commented Aug 14, 2023

Copy link
Copy Markdown
Contributor

The current this.req.destroyed = true hack that was added in 8b1923d actually breaks the behavior of 14.0.0.

Looking at v14.0.0's HTTP client's code, the abort method internally calls destroy.

destroy has an early termination when destroyed is already true. Setting destroyed = true before calling abort() stops Node.js from internally closing the socket, potentially leading to memory leaks.

It also breaks the behavior of abort() since Node.js deprecated it in favor of destroy() (as Node.js internally relies on destroy() logic to terminate the call.

Checklist

  • I have ensured my pull request is not behind the main or master branch of the original repository.
  • I have rebased all commits where necessary so that reviewing this pull request can be done without having to merge it first.
  • I have written a commit message that passes commitlint linting.
  • I have ensured that my code changes pass linting tests.
  • I have ensured that my code changes pass unit tests.
  • I have described my pull request and the reasons for code changes along with context if necessary.

@afharo afharo marked this pull request as ready for review August 14, 2023 11:31
@titanism titanism merged commit 4691583 into forwardemail:master Aug 15, 2023
@titanism

Copy link
Copy Markdown
Collaborator

v8.1.0 released which fixes this issue, thank you

npm install superagent@8.1.0

release notes @ https://github.com/ladjs/superagent/releases/tag/v8.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants