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

Patch memory leak with subscriptions when client abruptly terminates #668

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

Shane32
Copy link
Member

@Shane32 Shane32 commented Dec 14, 2021

@Shane32 Shane32 requested a review from sungam3r December 14, 2021 04:23
@Shane32 Shane32 self-assigned this Dec 14, 2021
@codecov-commenter
Copy link

Codecov Report

Merging #668 (d67b0fa) into master (db8484f) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #668   +/-   ##
=======================================
  Coverage   54.86%   54.86%           
=======================================
  Files          64       64           
  Lines        1582     1582           
  Branches      158      158           
=======================================
  Hits          868      868           
  Misses        660      660           
  Partials       54       54           
Impacted Files Coverage Δ
...ubscriptions.WebSockets/WebSocketReaderPipeline.cs 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db8484f...d67b0fa. Read the comment docs.

@Shane32
Copy link
Member Author

Shane32 commented Dec 14, 2021

@sungam3r I'm going to merge this so it is testable via GitHub Packages. Please let me know if you have any comments and I can make changes.

@Shane32 Shane32 changed the title Potential patch for memory leak Potential patch for subscriptions memory leak upon disconnection Dec 14, 2021
@Shane32 Shane32 changed the title Potential patch for subscriptions memory leak upon disconnection Patch memory leak with subscriptions when client abruptly terminates Dec 14, 2021
}
}
finally
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, the only change here is to move the if condition inside the try block, so the finally block executes regardless of the websocket state.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The finally block terminating the dataflow)

@Shane32 Shane32 merged commit 857a178 into master Dec 14, 2021
@Shane32 Shane32 deleted the memory_leak_patch branch December 14, 2021 17:10
@sungam3r sungam3r added the bugfix Pull request that fixes a bug label Dec 14, 2021
@sungam3r
Copy link
Member

ok

@sungam3r sungam3r added this to the v5.2 milestone Dec 14, 2021
@sungam3r sungam3r linked an issue Dec 14, 2021 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Probable memory leak in WebSocketReaderPipeline
3 participants