bugfix(client): panic: send on closed channel #179
Conversation
|
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/[email protected]/client.go:363
github.com/andreykaipov/goobs.(*Client).handleOpcodes(0xc0020c81a0, 0xc0013846c0)
/home/xaionaro/.gvm/pkgsets/go1.22.1/global/pkg/mod/github.com/andreykaipov/[email protected]/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/[email protected]/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
force-pushed
the
bugfix/panic_closed_channel
branch
from
4ec9a86
to
0442e5b
Compare
|
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. |
|
@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
force-pushed
the
bugfix/panic_closed_channel
branch
2 times, most recently
from
5b3e0ec
to
49c3129
Compare
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
force-pushed
the
bugfix/panic_closed_channel
branch
from
49c3129
to
314f152
Compare
Fix:
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.