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: handle client close event #8

Merged
merged 3 commits into from Sep 1, 2023

Conversation

didavid61202
Copy link
Contributor

@didavid61202 didavid61202 commented Sep 1, 2023

πŸ”— Linked issue

Downstream discussion: nuxt/nuxt#16046

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme, or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Proxy respond did not get 'abort/destroy' when the client close the request, causing situation in Server sent event where user trigger close from the clinet EventSource, but the server request did not trigger the close listen as expected.

Reproduction: https://stackblitz.com/edit/unjs-nitro-khtcjd?file=routes%2Fsse.ts
Server Sent Event(SSE) example code: https://gist.github.com/Atinux/05836469acca9649fa2b9e865df898a2

Downstream discussion: nuxt/nuxt#16046
Related issues: http-party/node-http-proxy#1520

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@codecov
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #8 (226388b) into main (f8cde14) will increase coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head 226388b differs from pull request most recent head e900b91. Consider uploading reports for the commit e900b91 to get more accurate results

@@            Coverage Diff             @@
##             main       #8      +/-   ##
==========================================
+ Coverage   64.98%   65.08%   +0.09%     
==========================================
  Files           8        8              
  Lines        1048     1051       +3     
  Branches       73       74       +1     
==========================================
+ Hits          681      684       +3     
  Misses        366      366              
  Partials        1        1              
Files Changed Coverage Ξ”
src/middleware/web-incoming.ts 70.55% <100.00%> (+0.49%) ⬆️

@pi0
Copy link
Member

pi0 commented Sep 1, 2023

Thanks for this PR. LGTM. It is worth to also check other upstream PRs for abort handling http-party/node-http-proxy#1580 http-party/node-http-proxy#1559

@pi0 pi0 changed the title fix: proxy client close event fix: handle client close event Sep 1, 2023
@pi0 pi0 merged commit d301da5 into unjs:main Sep 1, 2023
2 checks passed
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.

None yet

2 participants