`this.req.destroyed = true` in abort does not abort requests

[ulisesbocchio]

I just upgraded my codebase to latest version of superagent and noticed my unit tests breaking because of this: https://github.com/visionmedia/superagent/blob/29fd1f917a6cc78c9a2a9984269c06f8b4e63dcb/src/request-base.js#L505-L508

When I remove that line, they pass. I'd like to know what exactly are you referring to in your comment. In Node 16 LTS req.abort() works as expected without having to set the flag manually. Also, the documentation now discourages the use of req.abort() in favor of req.destroy() which I tested and works great too. For instance you could do req.destroy(new Error('Aborted')). To give you an example of how setting that flag does't work as expected here's some reference code:

const agentReq = superagent.get('http://localhost:5677/test');

const s = http.createServer((req, res) => {
    console.log(`<-- ${req.method} ${req.url}`);
    agentReq.abort();
    req.on('error', e => console.log(`XXX --> ${req.method} ${req.url} ${e.message}`));
    req.on('end', () => console.log(`--> ${req.method} ${req.url} 200`));
    setTimeout(() => {
        res.setHeader('Content-Type', 'application/json');
        res.writeHead(200);
        res.end('{ "test": "hello" }');
    }, 500);
});
s.listen(5677);

agentReq
    .then(
        r => console.log('Request Body:', r.body),
        e => console.log('Request Error:', e.message)
    )
    .finally(() => new Promise(resolve => s.close(resolve)));

Basically here there's a request going out to a server with a handler that's gonna abort that request while it's being handled. The handler has an "access logger" that logs out the result of the server side request, and the superagent request result is also logged when aborting the superagent request you'd expect this output:

<-- GET /test
Request Error: Aborted
XXX --> GET /test aborted

but instead you get:

<-- GET /test
Request Error: Aborted
--> GET /test 200

[afharo]

Same here... To be honest, I don't think that "fix" should have ever been applied. The comment in the commit points to the relevant piece of code in Node.js: setting destroyed: true essentially is stopping Node.js from actually destroying the underlying socket (potentially creating memory leaks).

I submitted the PR https://github.com/ladjs/superagent/pull/1774 to address this.

[titanism]

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