Repository navigation
Conversation
| // A response destroyed by the caller without an error (e.g. `res.destroy()`) emits neither | ||
| // 'end' nor 'error', only 'close'. Like a caller abort, this is not a failure of the request, | ||
| // so end the span with the status of the response. After 'end' or an error this is a no-op. | ||
| response.on('close', () => finishWithResponse(undefined, true)); |
There was a problem hiding this comment.
close is not only emitted when the caller destroys the response, but also when the connection dies/resets (server reset, req.destroy()).
Node calls res.destroy(ECONNRESET) and IncomingMessage only emits error if there is an 'error' listener (but it does not count errorMonitor).
If user code does not listen for error on the response, the span ends as ok (common with http.get(url, res => res.pipe(...))).
response.errored separates the two cases. It is null after res.destroy() and holds the error after a reset, even when Node did not emit that error:
response.on('close', () => finishWithResponse(response.errored));With this change, you can also remove the destroyedByCaller flag. Could you check that?
|
|
||
| let finished = false; | ||
| function finishWithResponse(error?: unknown): void { | ||
| function finishWithResponse(error?: unknown, destroyedByCaller = false): void { |
There was a problem hiding this comment.
If you use response.errored, you can remove the destroyedByCaller parameter.
| // A response destroyed by the caller without an error (e.g. `res.destroy()`) emits neither | ||
| // 'end' nor 'error', only 'close'. Like a caller abort, this is not a failure of the request, | ||
| // so end the span with the status of the response. After 'end' or an error this is a no-op. |
There was a problem hiding this comment.
Please make the comment shorter and correct it after the change, for example: "res.destroy() emits only close. errored separates a caller destroy from a connection error that was not emitted because nothing listens for error." Right now it says that close always means a caller destroy, which is not true.
|
Thanks, you were right. I reproduced it: with no
|
When the caller destroys an
http.get()response mid-body withres.destroy()(no error), thehttp.clientspan is never ended, so it is never sent. The response emits onlyclose, notendorerror, and the requestclosefallback is removed once the response arrives.This adds a response
closelistener that ends the span. Following #25260, a destroy by the caller is not treated as a failure: the span keeps the status of the response (ok for a 200, error for a 404).res.destroy(err)and a server closing the connection mid-body still set an error status, because they emiterrorbeforeclose. After a normalendor an error, the new listener is a no-op.Tests: caller destroy (200 and 404), caller destroy with an error, and a new server-abort case. The caller-destroy test times out without the fix. All 121 http tests pass. Reproduced on @sentry/node 11.8.0.
Closes #25469