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 subscriber close #877

Merged
merged 1 commit into from
Oct 7, 2022
Merged

Conversation

AndrewSisley
Copy link
Contributor

Relevant issue(s)

Resolves #872

Description

Much simpler solution to #873 as pointed out by Fred :)

How has this been tested?

Tested manually.

Specify the platform(s) on which this was tested:

  • Debian Linux

@AndrewSisley AndrewSisley added bug Something isn't working area/db-system Related to the core system related components of the DB action/no-benchmark Skips the action that runs the benchmark. labels Oct 7, 2022
@AndrewSisley AndrewSisley added this to the DefraDB v0.4 milestone Oct 7, 2022
@AndrewSisley AndrewSisley requested a review from a team October 7, 2022 14:15
@AndrewSisley AndrewSisley self-assigned this Oct 7, 2022
@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Merging #877 (4b354a9) into develop (fda90b5) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #877      +/-   ##
===========================================
- Coverage    59.81%   59.80%   -0.01%     
===========================================
  Files          157      157              
  Lines        17425    17425              
===========================================
- Hits         10422    10421       -1     
- Misses        6066     6067       +1     
  Partials       937      937              
Impacted Files Coverage Δ
events/simple.go 86.76% <100.00%> (+2.94%) ⬆️
net/peer.go 30.92% <100.00%> (-0.99%) ⬇️

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the fix :)

@@ -70,14 +70,14 @@ func (c *simpleChannel[T]) Close() {
if c.isClosed {
return
}
c.isClosed = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

praise: this change fixes the race condition :) Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see, what you meant in the original now - this was also (somewhat sneakily) added in that PR too :)

@AndrewSisley AndrewSisley merged commit f9747f1 into develop Oct 7, 2022
@AndrewSisley AndrewSisley deleted the sisley/fix/I872-event-close-c branch October 7, 2022 14:44
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. area/db-system Related to the core system related components of the DB bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Events: Closing subscription channel spams the logs
2 participants