Skip to content
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

this.req.destroyed = true in abort does not abort requests #1741

Closed
ulisesbocchio opened this issue Sep 23, 2022 · 2 comments
Closed

this.req.destroyed = true in abort does not abort requests #1741

ulisesbocchio opened this issue Sep 23, 2022 · 2 comments

Comments

@ulisesbocchio
Copy link

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
Copy link
Contributor

afharo commented Aug 14, 2023

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 #1774 to address this.

@titanism
Copy link
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

No branches or pull requests

3 participants