ladjs / superagent

Ajax for Node.js and browsers (JS HTTP client). Maintained for @forwardemail, @ladjs, @spamscanner, @breejs, @cabinjs, and @lassjs.
https://ladjs.github.io/superagent/
MIT License
16.58k stars 1.33k forks source link

"superagent: double callback bug" with timeout #1801

Open gramakri opened 5 months ago

gramakri commented 5 months ago

Describe the bug

Node.js version: v18.16.0

OS version: ubuntu 22.04

Description: When a request aborts with a timeout, it calls the callback twice resulting in a message being printed on the console "superagent: double callback bug"

Expected behavior

Should not print the console message and should not call the callback twice

Code to reproduce

const superagent = require('superagent');

(async function () {
    try {
        await superagent.get('https://some/slow/api').timeout(20000);
    } catch (error) {
        console.log(error);
    }
})();

Output (with DEBUG=*) :

  superagent GET https://some/slow/api +0ms
  superagent GET https://some/slow/api -> 200 +5s

Error: Timeout of 20000ms exceeded
    at RequestBase._timeoutError (/home/yellowtent/box/node_modules/superagent/lib/request-base.js:712:17)
    at Timeout.<anonymous> (/home/yellowtent/box/node_modules/superagent/lib/request-base.js:727:12)
    at listOnTimeout (node:internal/timers:569:17)
    at process.processTimers (node:internal/timers:512:7) {
  timeout: 20000,
  code: 'ECONNABORTED',
  errno: 'ETIME',
  response: undefined
}
superagent: double callback bug

Checklist

gramakri commented 5 months ago

Unfortunately, this timeout error doens't happen always but most of the time. I debugged this a bit and the second callback error that causes the message is:

Error: aborted
    at connResetException (node:internal/errors:717:14)
    at TLSSocket.socketCloseListener (node:_http_client:462:19)
    at TLSSocket.emit (node:events:525:35)
    at node:net:322:12
    at TCP.done (node:_tls_wrap:588:7) {
  code: 'ECONNRESET'
}

The timeout() code calls abort which maybe somehow ends up with an 'error' event with reset ? This matches the behavior outlined in https://nodejs.org/api/http.html#httprequesturl-options-callback which says:

In the case of a premature connection close after the response is received, the following events will be emitted in the following order:

    'socket'
    'response'
        'data' any number of times, on the res object
    (connection closed here)
    'aborted' on the res object
    'error' on the res object with an error with message 'Error: aborted' and code 'ECONNRESET'
    'close'
    'close' on the res object