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: destroy request object after successful response (issue #1489) #2187

Merged
merged 1 commit into from
Dec 7, 2022

Conversation

nduthoit
Copy link

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • (n/a) If it's a new feature, I have included documentation updates in both the README and the types.

Based on my testing this fixes #1489 for v11: late ECONNRESET errors no longer causes the "onCancel handler was attached after the promise settled" error. (I used this gist for testing).

It mimics what is done in v12 by calling request.destroy() after a successful response right before the promise is resolved.

I tried to add a test case where the server would reset the TCP connection and cause an ECONNRESET error after sending a success response but that proved to be challenging to do using an Express server. I also tried using a Python server executed from the test case but it proved to be too brittle to be a good test case. Therefore I landed on a simpler test case that ensures that the request has been destroyed once the promise has successfully resolved.

@nduthoit nduthoit changed the title fix: destroy request object after successful response fix: destroy request object after successful response (issue #1489) Nov 24, 2022
@sindresorhus sindresorhus merged commit 2d1497e into sindresorhus:v11 Dec 7, 2022
@sindresorhus
Copy link
Owner

@nduthoit Looks like the v11 branch no longer builds. So you need to fix the build issues if you want us to do a release.

@sindresorhus
Copy link
Owner

https://github.com/sindresorhus/got/releases/tag/v11.8.6

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.

3 participants