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

Listens to response close event for aborted request trigger on http 1 #1181

Closed
wants to merge 1 commit into from

Conversation

lourd
Copy link

@lourd lourd commented Aug 22, 2024

Fixes #1025.

This breaks some backwards compatibility. Up until now the "aborted" state of the request was always set as soon as the body of the request had been read. This is undesired, buggy behavior, as #1025 documents. For any applications using the abort controller's signal, they would be aborted every time!

The Node.js documentation on the request close event is misleading at best, inaccurate at worst, as discussed in nodejs/node#40775. Nevertheless, listening to the close event of the response for aborted client requests is the functionally correct implementation. For example, see New Relic changing their instrumentation library to do so newrelic/node-newrelic#1510

The exported universalRequestFromNodeRequest function now requires the response parameter as well.

Signed-off-by: Louis DeScioli <louis.descioli@gmail.com>
@srikrsna-buf
Copy link
Member

srikrsna-buf commented Sep 9, 2024

#1025 has been addressed in #1218, the solution is mostly similar to this PR but keeps the current error/api. Thank you for your work on this.

@lourd
Copy link
Author

lourd commented Sep 9, 2024

You're welcome, thanks @srikrsna-buf!

When do y'all plan on publishing the next release?

@lourd lourd deleted the fix/http1-close branch September 9, 2024 13:49
@timostamm
Copy link
Member

The next release will be out by the end of the week, or early next week.

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

Successfully merging this pull request may close these issues.

Requests appear aborted before handler over HTTP/1.1
3 participants