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: issue #743 - close channel on Request 'aborted' event #744

Merged
merged 1 commit into from
Nov 28, 2022

Conversation

orinem
Copy link
Contributor

@orinem orinem commented Nov 28, 2022

PR Checklist

  • Unit Tests have been added for new changes

What are you changing?

Ensure that the channel is closed if an API request is aborted (for example: back, reload etc. in the browser with the sample UI).

Anything the reviewer should know when reviewing this PR?

This is a placeholder really. See the issue for details about a 'better' fix to avoid the potentially moving target of which events are emitted by which object and when.

Note that the aborted event seems to be deprecated with later versions of nodejs. They suggest using 'close' instead, but that is called for every request. If that is the case, then it alone could be used and there would be no need to listen for 'finish' or 'close' on the response.

The unit tests have been improved test to ensure CloseChannel is called and listeners removed after a request 'completes'.

…rted' event

Ensure channel is closed if an API request is aborted.
Improved afterResponse test to ensure CloseChannel is called.
@codecov
Copy link

codecov bot commented Nov 28, 2022

Codecov Report

Merging #744 (0b041d0) into main (de565d1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 0b041d0 differs from pull request most recent head 41bd3ca. Consider uploading reports for the commit 41bd3ca to get more accurate results

@@           Coverage Diff           @@
##             main     #744   +/-   ##
=======================================
  Coverage   93.65%   93.66%           
=======================================
  Files          78       78           
  Lines        3293     3298    +5     
  Branches      527      528    +1     
=======================================
+ Hits         3084     3089    +5     
  Misses        209      209           
Impacted Files Coverage Δ
src/server/webserver.ts 84.76% <100.00%> (+0.76%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@rsdmike rsdmike linked an issue Nov 28, 2022 that may be closed by this pull request
@rsdmike rsdmike enabled auto-merge (rebase) November 28, 2022 16:40
@rsdmike rsdmike merged commit c29d80d into open-amt-cloud-toolkit:main Nov 28, 2022
@orinem orinem deleted the issue_743 branch November 28, 2022 16:51
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.

Aborted requests leak AMT Channels
2 participants