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.

Sign up for GitHub

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

Jump to bottom

CASSGO-41 Fix deadlock in refresh debouncer stop #1767

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

CASSGO-41 Fix deadlock in refresh debouncer stop #1767

wants to merge 1 commit into from

Conversation

joao-r-reis
Copy link
Contributor

fix #1752

@joao-r-reis joao-r-reis mentioned this pull request Jun 6, 2024
Open
kevinkyyro
kevinkyyro reviewed Jul 30, 2024
View reviewed changes
host_source_test.go
// https://github.com/gocql/gocql/issues/1752
func TestRefreshDebouncer_DeadlockOnStop(t *testing.T) {
// there's no way to guarantee this bug manifests because it depends on which `case` is picked from the `select`
// with 4 iterations of this test the deadlock would be hit pretty consistently
Copy link

Choose a reason for hiding this comment

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

Does this comment indicate that you were reliably reproducing it without the fix in place or is it just speculating that 4 should be enough to hit it?

Copy link
Contributor Author

@joao-r-reis joao-r-reis Jul 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I was reliably reproducing it with 4 without the fix in place

kevinkyyro
kevinkyyro approved these changes Aug 5, 2024
View reviewed changes
Copy link

@kevinkyyro kevinkyyro left a comment

Choose a reason for hiding this comment

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

Not sure if my review helps get this merged, but I checked out the branch and verified that the test reliably reproduces the bug without the fix and passes with the fix. Thanks!

@joao-r-reis joao-r-reis changed the title Fix deadlock in refresh debouncer stop CASSGO-41 Fix deadlock in refresh debouncer stop Nov 25, 2024
jameshartig
jameshartig reviewed Jan 31, 2025
View reviewed changes
host_source.go
refreshFn func() error
quitCtxFn context.CancelFunc
Copy link
Contributor

@jameshartig jameshartig Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I would say this is a bit of an abuse of contexts. Instead I would keep the old behavior but just change a small thing. Add another chan struct{} here called done that is defer close(done) in flusher. Then in stop() keep what was there but don't write to quit (I'm not really sure what the purpose of that was anyways) and just close it. Then do <-done to wait for flusher to stop.

jameshartig
jameshartig requested changes Jan 31, 2025
View reviewed changes
Copy link
Contributor

@jameshartig jameshartig left a comment

Choose a reason for hiding this comment

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

I think what was there was pretty close just need to make some small changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jameshartig jameshartig jameshartig requested changes

@kevinkyyro kevinkyyro kevinkyyro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CASSGO-41 Deadlock in refreshDebouncer when reconnection fails
3 participants
@joao-r-reis @jameshartig @kevinkyyro