Regression handling multiple request errors

[mastermatt]

Describe the bug

• Node.js version: 10, 12, 14
• OS & version: macOS 10.15.4

Starting with v11.0, emitting more than one error on the ClientRequest causes an unhandled error event.

Code to reproduce

got('http://example.com')
  .on('request', req => {
    req.once('socket', () => {
      req.emit('error', new Error("foo bar 1"))
      req.emit('error', new Error("foo bar 2"))
    })
  })
  .catch(reason => {
    console.log('caught', reason.message)
  })

Actual behavior

    throw er; // Unhandled 'error' event
    ^

Error: foo bar 2
    ...

Expected behavior

Previous behavior was to reject the promise with the first error, but each subsequent error would still run through the beforeError hook and propagate the event.
Not sure if that is the right behavior, but it probably is.
Regardless, looking at the changelog, this new behavior seems to be an unintended change.

I'm sure @szmarczak will know off hand, but this seems to have been caused by the "Rewrite Got" aligning on using request.once instead of using request.on in request-as-event-emitter.ts.

One thing to note about the 'code to reproduce' above is that the errors are emitted between the socket and response events. This was identified because it broke Nock's test suite when upgrading to Got 11. nock/nock#1992
Nock emits the "Nock: No match for request" error between those two events, Got then calls abort on the ClientRequest which emits a "socket hang up" error.
The same behavior can seen with the 'code to reproduce' by commented out the second call to emit.

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[szmarczak]

Starting with v11.0, emitting more than one error on the ClientRequest causes an unhandled error event.

That is correct. Please note, that starting Node.js 14.0.0 the recommended approach to req.destroy(...) and not req.emit('error', ...). Streams that have errored should be destroyed.

[szmarczak]

this new behavior seems to be an unintended change

It is intended. It leads to more strict behavior. But you're right, it should've been mentioned in the changelog.

https://nodejs.org/api/stream.html#stream_event_error

[szmarczak]

Please see the following: nodejs/node#32995 (comment)

[mastermatt]

Thanks for the response.

For other folks coming here, the relevant bit of the docs linked above for stream.Writable "error" event:

After 'error', no further events other than 'close' should be emitted (including 'error' events).

---

This question might be better suited for the Node issue thread, but I'm hoping to pick @szmarczak brain.
If destroy is preferred over emitting an error on ClientRequest, why are conn reset errors emitted? (example)

Because the following throws an uncaught error event, even in v14.

const req = http.get("http://example.com")
req.on("socket", () => req.destroy())

[szmarczak]

If destroy is preferred over emitting an error on ClientRequest, why are conn reset errors emitted?

Because the Node.js maintainers haven't fixed it yet / didn't have the time to do so. Note that it is a big project. You can find many bugs in Node.js, not only this. /cc @ronag

[ronag]

If destroy is preferred over emitting an error on ClientRequest, why are conn reset errors emitted? (example )

Because Node has a lot of old code which is not trivial to re-write to use pragmatic streams. It should use destroy(err) but we haven't yet managed to refactor this part of the code. It is work in progress.

If you look at the changlog for v13 and v14 we have fixed a lot of these cases in core and are working to sort out even more.

[ronag]

Because the following throws an uncaught error event, even in v14.

That works as expected. Why is this strange?

destroy() does not in any way guarantee that no error will be emitted before 'close' is emitted.

[mastermatt]

I understand the work in progress, makes perfect sense.

That works as expected. Why is this strange?

With Node's current implementation, I agree that an uncaught error is expected.
I just want to clarify that once the work in progress is finished, there won't be an uncaught error anymore.
My understanding is that req.destroy() is the preferred way to end/stop a request without an error, however, currently (v14.2) calling destroy before the response event implicitly emits an error.

[ronag]

My understanding is that req.destroy() is the preferred way to end/stop a request without an error,

You can't end/stop a request and be guaranteed to not receive an error.

[mastermatt]

You can't end/stop a request and be guaranteed to not receive an error.

Understood. How do I end a request without causing an error?

[ronag]

Understood. How do I end a request without causing an error?

request.end(), but that does not guarantee you won't get any error.

[mastermatt]

Sorry, I'm not worried about guaranteeing an error or multiple errors won't happen, nor if they're uncaught if they do happen.
I also didn't mean to use end in this case, as request.end() will continue the request/response flow.
I want to stop a request in its tracks (abort/kill/term) and not have the request emit an error for that action.

[ronag]

I want to stop a request in its tracks (abort/kill/term) and not have the request emit an error for that action.

Not possible unfortunately.

[mastermatt]

Does the design of the work in progress (using destroy for the conn resets) allow for this to be possible? Or is the current behavior desired?

[ronag]

Does the design of the work in progress (using destroy for the conn resets) allow for this to be possible? Or is the current behavior desired?

The current behavior is desired.

[mastermatt]

Thanks for your time @ronag