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

bugfix(client): panic: send on closed channel #179

Merged

Conversation

xaionaro
Copy link
Contributor

Fix:

panic: send on closed channel

goroutine 751872 [running]:
github.com/andreykaipov/goobs.(*Client).writeEvent(...)
    /home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/goobs@v1.4.1/client.go:363
github.com/andreykaipov/goobs.(*Client).handleOpcodes(0xc0020c81a0, 0xc0013846c0)
    /home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/goobs@v1.4.1/client.go:338 +0x5a5
created by github.com/andreykaipov/goobs.(*Client).connect in goroutine 751658
    /home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/goobs@v1.4.1/client.go:200

Essentially by design we should close a channel only after we finished all possible writing to it. Thus moving the close statement of channel IncomingResponses to be called right after the writer to the channel is finished.

@andreykaipov
Copy link
Owner

Hi @xaionaro - thanks for the PRs! Sorry it took a while. I didn't have time to figure out why the CI was failing in this repo. But now that's fixed in #181, can you rebase your PRs or pull in the latest changes from main to retrigger the CI? I wanna make sure that passes before merging them in! 🙏

Alternatively I think there's a checkbox somewhere that allows you to allow edits from maintainers and I can rebase for you.

Fix:

    panic: send on closed channel

    goroutine 751872 [running]:
    github.com/andreykaipov/goobs.(*Client).writeEvent(...)
        /home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/goobs@v1.4.1/client.go:363
    github.com/andreykaipov/goobs.(*Client).handleOpcodes(0xc0020c81a0, 0xc0013846c0)
        /home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/goobs@v1.4.1/client.go:338 +0x5a5
    created by github.com/andreykaipov/goobs.(*Client).connect in goroutine 751658
        /home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/goobs@v1.4.1/client.go:200

Essentially by design we should close a channel only after we finished
all possible writing to it. Thus moving the close statement of channel
IncomingResponses to be called right after the writer to the channel is
finished.
@xaionaro xaionaro force-pushed the bugfix/panic_closed_channel branch from 4ec9a86 to 0442e5b Compare October 25, 2024 14:01
@xaionaro xaionaro closed this Oct 25, 2024
@xaionaro
Copy link
Contributor Author

xaionaro commented Oct 25, 2024

Wait, let me re-figure-out why I wrote the patch the way I did it (to make sure I haven't missed anything). Will re-open after the recheck.

@xaionaro xaionaro reopened this Oct 25, 2024
@xaionaro
Copy link
Contributor Author

xaionaro commented Oct 25, 2024

@andreykaipov It seems like my fix was correct (added a comment into the code), but a bit incomplete. Added a one more commit to fix some other potential (but less likely) race conditions.

I've also rebased my project on top of these commits, so to be extra safe we can wait a week and see if it works fine.

@xaionaro xaionaro mentioned this pull request Oct 25, 2024
client.go Outdated Show resolved Hide resolved
@xaionaro xaionaro force-pushed the bugfix/panic_closed_channel branch 2 times, most recently from 5b3e0ec to 49c3129 Compare October 25, 2024 16:47
There were three problems:
1. Instead of closing c.Disconnect we were writing a single event there.
   Thus if somebody will try to read from the channel the second time,
   they'll get blocked, and thus SendRequest will go into the `default`
   section of the `select`, which is potentially a panic if `c.Opcodes`
   is already closed.
2. The `close` statements in `markDisconnect` are executed without
   locking c.client.mutex. As a result there is a possible race
   condition that for example c.IncomingEvents is closed,
   but c.client.Disconnected is not (which may lead to a panic).
3. The place of the closure of c.client.IncomingResponses is
   confusing after 0442e5b.

The problems are now fixed:
1. Now we just close `c.Disconnect` instead of writing an event into it.
2. Now the `close` statements are made atomic via c.client.mutex.
3. Now we have a comment explaining the closure of c.client.IncomingResponses
@xaionaro xaionaro force-pushed the bugfix/panic_closed_channel branch from 49c3129 to 314f152 Compare October 25, 2024 16:48
client.go Show resolved Hide resolved
client.go Show resolved Hide resolved
@andreykaipov andreykaipov merged commit 42459fc into andreykaipov:main Oct 28, 2024
8 checks passed
@andreykaipov andreykaipov added the bug Something isn't working label Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants