Skip to content

Commit

Permalink
fix: added abort fix logic for all Node versions
Browse files Browse the repository at this point in the history
  • Loading branch information
niftylettuce committed Jan 7, 2022
1 parent 7ac9d42 commit 8b1923d
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 2 deletions.
6 changes: 5 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
"@commitlint/config-conventional"
]
},
"engines": {
"node": ">=6.4.0 <13|>=14"
},
"contributors": [
"Kornel Lesiński <kornel@geekhood.net>",
"Peter Lyons <pete@peterlyons.com>",
Expand Down Expand Up @@ -57,10 +60,11 @@
"express": "^4.17.1",
"express-session": "^1.17.2",
"fixpack": "^4.0.0",
"get-port": "4.2.0",
"husky": "^7.0.4",
"lint-staged": "^12.1.2",
"marked": "^2.0.0",
"mocha": "3.5.3",
"mocha": "6.2.2",
"multer": "^1.4.3",
"nyc": "^15.1.0",
"remark-cli": "^10.0.1",
Expand Down
26 changes: 25 additions & 1 deletion src/request-base.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const semver = require('semver');

/**
* Module of mixed-in functions shared between node and client code
*/
Expand Down Expand Up @@ -496,7 +498,29 @@ RequestBase.prototype.abort = function () {

this._aborted = true;
if (this.xhr) this.xhr.abort(); // browser
if (this.req) this.req.abort(); // node
if (this.req) {
// Node v13 has major differences in `abort()`
// https://github.com/nodejs/node/blob/v12.x/lib/internal/streams/end-of-stream.js
// https://github.com/nodejs/node/blob/v13.x/lib/internal/streams/end-of-stream.js
// https://github.com/nodejs/node/blob/v14.x/lib/internal/streams/end-of-stream.js
// (if you run a diff across these you will see the differences)
//
// References:
// <https://github.com/nodejs/node/issues/31630>
// <https://github.com/visionmedia/superagent/pull/1084/commits/dc18679a7c5ccfc6046d882015e5126888973bc8>
//
// Thanks to @shadowgate15 and @niftylettuce
if (semver.gte(process.version, 'v13.0.0') && semver.lt(process.version, 'v14.0.0')) {
// Note that the reason this doesn't work is because in v13 as compared to v14
// there is no `callback = nop` set in end-of-stream.js above
throw new Error('Superagent does not work in v13 properly with abort() due to Node.js core changes');
} else if (semver.gte(process.version, 'v14.0.0')) {
// We have to manually set `destroyed` to `true` in order for this to work
// (see core internals of end-of-stream.js above in v14 branch as compared to v12)
this.req.destroyed = true;
}
this.req.abort(); // node
}
this.clearTimeout();
this.emit('abort');
return this;
Expand Down

1 comment on commit 8b1923d

@Gabba90
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @niftylettuce,

I am not getting why we should set destroyed to true to abort the request.
By doing so it seems the underlying socket will not be destroyed here since we return earlier (here).

I am probably missing something.

Many thanks for considering this comment.

Please sign in to comment.