Skip to content

Do not close channel after subscription cancellation#428

Merged
tzdybal merged 2 commits intomainfrom
jbowen93/no-close
Jul 5, 2022
Merged

Do not close channel after subscription cancellation#428
tzdybal merged 2 commits intomainfrom
jbowen93/no-close

Conversation

@jbowen93
Copy link
Copy Markdown
Contributor

@jbowen93 jbowen93 commented Jun 6, 2022

No description provided.

@jbowen93 jbowen93 force-pushed the jbowen93/no-close branch from 5f4e18d to 91d93d8 Compare June 6, 2022 15:43
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jun 6, 2022

Codecov Report

Merging #428 (cd7143a) into main (bfa0fa4) will decrease coverage by 0.02%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #428      +/-   ##
==========================================
- Coverage   55.22%   55.20%   -0.03%     
==========================================
  Files          48       48              
  Lines        8798     8796       -2     
==========================================
- Hits         4859     4856       -3     
- Misses       3188     3189       +1     
  Partials      751      751              
Impacted Files Coverage Δ
rpc/json/service.go 65.00% <ø> (-0.65%) ⬇️
rpc/json/ws.go 61.90% <0.00%> (-1.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 bfa0fa4...cd7143a. Read the comment docs.

@tzdybal tzdybal linked an issue Jun 13, 2022 that may be closed by this pull request
@tzdybal tzdybal force-pushed the jbowen93/no-close branch from f69c043 to b732774 Compare July 4, 2022 20:27
@tzdybal
Copy link
Copy Markdown
Contributor

tzdybal commented Jul 4, 2022

Rebased PR on top of current main. Closing a channel should be used for signaling, so in original code it was used incorrectly. (Close is not required to free resources, it's done by GC anyways).

@tzdybal tzdybal marked this pull request as ready for review July 4, 2022 20:41
@tzdybal tzdybal self-assigned this Jul 4, 2022
@tzdybal tzdybal changed the title Jbowen93/no close Do not close channel after subsciption cancellation Jul 4, 2022
@tzdybal tzdybal changed the title Do not close channel after subsciption cancellation Do not close channel after subscription cancellation Jul 4, 2022
Comment thread rpc/json/ws.go Outdated
Comment thread rpc/json/ws.go Outdated
@tzdybal tzdybal force-pushed the jbowen93/no-close branch from 013278a to 2df6392 Compare July 5, 2022 11:13
@tzdybal tzdybal requested a review from adlerjohn July 5, 2022 13:42
Copy link
Copy Markdown
Contributor

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

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

utACK

@tzdybal tzdybal merged commit d1fe08d into main Jul 5, 2022
@tzdybal tzdybal deleted the jbowen93/no-close branch July 5, 2022 19:36
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.

Panic when subscription is canceled

4 participants