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

[fix] Request not being properly disposed when it encounters an error #1827

Open
3 tasks done
dipunm opened this issue Jan 30, 2025 · 1 comment
Open
3 tasks done
Labels

Comments

@dipunm
Copy link

dipunm commented Jan 30, 2025

Describe the bug

Node.js version: 18.20.4

OS version: MacOs Sequoia 15.2

Description: After causing the request to crash by supplying undefined headers, the request is not properly closed and this causes a dangling handle that lasts for a few seconds.

Actual behavior

Expected behavior

The request should be ended, even if there are unexpected exceptions.

Code to reproduce

const superagent = require('superagent');

const r = superagent.get('https://www.google.com');
try {
        r.set({ 'x-header': undefined }).end((err, result) => console.log('completed', err, result));
//      r.set({ 'x-header': undefined }).then(result => console.log('completed', result)).catch(err => {
                console.error('rejected', err);
//              r.req.end();
        });
} catch(e) {
        console.log('caught', e);

//      r.req.end();
}

This code snippet is a setup for two scenarios:

  1. The current setup will use the end() syntax and when the error is caught, the application will hang for a few seconds and then exit.
  2. If you uncomment the r.req.end() line inside the catch, the application will exit immediately.
  3. If we comment out the first line inside the try block and uncomment the second line, we now are testing the promise based method. This also hangs before allowing the application to exit.
  4. If we comment out the r.req.end() line inside the .catch() handler, then the application exits immediately.

Checklist

  • I have searched through GitHub issues for similar issues.
  • I have completely read through the README and documentation.
  • I have tested my code with the latest version of Node.js and this package and confirmed it is still not working.
@dipunm dipunm added the Bug label Jan 30, 2025
@dipunm
Copy link
Author

dipunm commented Jan 31, 2025

To make it even clearer, here are a few scenarios, each can be put into a js file and executed within a project folder that has superagent installed as a dependency:

1

const superagent = require('superagent');

const r = superagent.get('https://www.google.com');
try {
        r.set({ 'x-header': undefined }).end((err, result) => console.log('completed', err, result));
                console.error('rejected', err);
        });
} catch(e) {
        console.log('caught', e);
}

console.log('dangling request?', process.getActiveResourcesInfo().includes('TCPSocketWrap'));

Here, the library will throw an exception, the try/catch will catch and log it, and there WILL be a dangling request causing the application to hang for a moment before completing. 🕐

Also, note that the end method does not capture the error and pass it to the callback as expected which is why we needed a try/catch block. ❌

2

const superagent = require('superagent');

const r = superagent.get('https://www.google.com');
try {
        r.set({ 'x-header': undefined }).end((err, result) => console.log('completed', err, result));
} catch(e) {
        console.log('caught', e);
        r.req.end();
}

console.log('dangling request?', process.getActiveResourcesInfo().includes('TCPSocketWrap'));

Here, the library will throw an exception, the try/catch will catch and log it, and there WILL NOT be a dangling request and the application will complete immediately. ✅

3

const superagent = require('superagent');

const r = superagent.get('https://www.google.com');

r.set({ 'x-header': undefined }).then(result => console.log('completed', result)).catch(err => {
        console.error('rejected', err);
        console.log('dangling request?', process.getActiveResourcesInfo().includes('TCPSocketWrap'));
});

Here we are using the promise logic, it appears that the exception is rejected correctly. The application will still hang for a while before exiting and the dangling request will still be present. 🕐

4

const superagent = require('superagent');

const r = superagent.get('https://www.google.com');

r.set({ 'x-header': undefined }).then(result => console.log('completed', result)).catch(err => {
        console.error('rejected', err);
        r.req.end();
        console.log('dangling request?', process.getActiveResourcesInfo().includes('TCPSocketWrap'));
});

In this scenario, there is no dangling request ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant