Skip to content

fix: avoid missed wakeups in rpc stream subscribe#1184

Open
luchenqun wants to merge 1 commit into
cosmos:mainfrom
luchenqun:fix/stream-subscribe-race
Open

fix: avoid missed wakeups in rpc stream subscribe#1184
luchenqun wants to merge 1 commit into
cosmos:mainfrom
luchenqun:fix/stream-subscribe-race

Conversation

@luchenqun

Copy link
Copy Markdown

Description

Fix the rpc stream subscription race that could miss the final wakeup in blocking reads.
Initialize subscription offsets from the current tail so subscribers only receive new items.
Capture the cond channel before releasing the read lock to avoid dropped broadcasts.

Verification

go test -count=20 -race -tags=test ./rpc/stream -run 'TestStream(ReadBlocking|Subscribe)'
go test -race -tags=test ./rpc/stream

@luchenqun luchenqun requested a review from a team as a code owner May 14, 2026 09:01
@greptile-apps

greptile-apps Bot commented May 14, 2026

Copy link
Copy Markdown
Contributor

PR author is not in the allowed authors list.

@aljo242

aljo242 commented May 26, 2026

Copy link
Copy Markdown
Contributor

two concerns before merge:

  1. stream.go:74: changing offset = -1 to offset = s.lastID() is a silent behavior change. any caller relying on replay-from-beginning semantics will silently stop receiving historical items. please document this in the changelog and confirm no existing Subscribe call sites depend on the old behavior.

  2. no regression tests. the missed-wakeup race fix and the new offset behavior both need test coverage. the pr description describes manual verification steps but those do not prevent future regressions. please add a test that exercises concurrent Add + ReadBlocking under -race to pin the race fix, and a test for the new subscribe-from-tail behavior.

minor: Channel() in cond.go is exported but only used internally by ReadBlocking. unexport it.

@aljo242 aljo242 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.

silent behavior change in Subscribe offset, missing regression tests, unnecessary export of Channel().

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.

2 participants