Skip to content

prevent deadlocks during event stream teardown - #292

Merged
mjdwitt merged 1 commit into
mainfrom
fix-qemu-event-teardown-race
Aug 14, 2026
Merged

prevent deadlocks during event stream teardown#292
mjdwitt merged 1 commit into
mainfrom
fix-qemu-event-teardown-race

Conversation

@mjdwitt

@mjdwitt mjdwitt commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

previous to this change, Libvirt.SubscribeQEMUEvents sometimes failed
to shut down on context cancellation due to a race between two
incorrectly-ordered defers:

ctx, cancel := context.WithCancel(ctx)
defer cancel()
defer l.unsubscribeQEMUEvents(stream)
defer stream.Shutdown()

the deferred stream.Shutdown would run first, allowing a small window
where the stream was closed but still registered to receive events. if an
event arrived in that window, the shared socket-reader goroutine (which
routes every incoming event and RPC response for the connection) would
block forever inside Stream.Push, since nothing was left to drain the
stream's queue. because that goroutine is shared across the whole
connection, blocking it wedged every other in-flight RPC too - including
the deregister call l.unsubscribeQEMUEvents(stream) was waiting on, and
Disconnect's own close call. this change reorders those defers so the
stream is always unsubscribed before its local reader is shut down,
closing the window.

furthermore, this also adds a guard in Stream.Push that prevents
hanging if the stream has already been shut down via context
cancellation.

@mjdwitt
mjdwitt requested a review from a team as a code owner August 13, 2026 23:52

@mpontillo mpontillo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good; let's credit the original author (as in the AUTHORS change in #190), otherwise lgtm!

previous to this change, `Libvirt.SubscribeQEMUEvents` sometimes failed
to shut down on context cancellation due to a race between two
incorrectly-ordered defers:

```go
ctx, cancel := context.WithCancel(ctx)
defer cancel()
defer l.unsubscribeQEMUEvents(stream)
defer stream.Shutdown()
```

the deferred `stream.Shutdown` would run first, allowing a small window
where the stream was closed but still registered to receive events. if an
event arrived in that window, the shared socket-reader goroutine (which
routes every incoming event and RPC response for the connection) would
block forever inside `Stream.Push`, since nothing was left to drain the
stream's queue. because that goroutine is shared across the whole
connection, blocking it wedged every other in-flight RPC too - including
the deregister call `l.unsubscribeQEMUEvents(stream)` was waiting on, and
`Disconnect`'s own close call. this change reorders those defers so the
stream is always unsubscribed before its local reader is shut down,
closing the window.

furthermore, this also adds a guard in `Stream.Push` that prevents
hanging if the stream has already been shut down via context
cancellation.

Co-authored-by: Nicholas Kwan <ready725@gmail.com>
@mjdwitt
mjdwitt force-pushed the fix-qemu-event-teardown-race branch from 29e0e59 to 5166039 Compare August 14, 2026 18:41
@mjdwitt
mjdwitt merged commit 1a83157 into main Aug 14, 2026
5 checks passed
@mpontillo
mpontillo deleted the fix-qemu-event-teardown-race branch August 14, 2026 19:05
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.

3 participants