Skip to content

subscription: fix producer goroutine leak on context cancel - #760

Open
hsieh wants to merge 1 commit into
graphql-go:masterfrom
hsieh:master
Open

subscription: fix producer goroutine leak on context cancel#760
hsieh wants to merge 1 commit into
graphql-go:masterfrom
hsieh:master

Conversation

@hsieh

@hsieh hsieh commented Aug 26, 2026

Copy link
Copy Markdown

The producer in ExecuteSubscription sent results with a bare unbuffered send that did not select on p.Context.Done(). When a subscriber unsubscribes or disconnects and stops reading resultChannel, the producer blocks forever on the send; cancelling the context cannot unblock it because the goroutine is not in a select at that moment. This leaks one goroutine per unsubscribed subscription.

Wrap the send in a select that also observes p.Context.Done(), so a cancelled context lets the producer exit and close resultChannel even with no active receiver.

Add TestProducerExitsOnContextCancel as a regression test.

Summary by CodeRabbit

  • Bug Fixes

    • Improved subscription handling when a request is cancelled, preventing stalled operations and ensuring result channels close cleanly.
    • Subscription producers now exit promptly instead of remaining active when no receiver is available.
  • Tests

    • Added regression coverage for subscription cancellation and producer shutdown behavior.

@coderabbitai

coderabbitai Bot commented Aug 26, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 23bc8ed6-afaf-4b9d-adfa-a52d500df25b

📥 Commits

Reviewing files that changed from the base of the PR and between 6acef35 and 1a4d863.

📒 Files selected for processing (2)
  • subscription.go
  • subscription_leak_test.go

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.


📝 Walkthrough

Walkthrough

Subscription result forwarding now checks the execution context before each send. A regression test verifies that cancelling a blocked subscription closes the result channel and exits the producer.

Changes

Subscription cancellation handling

Layer / File(s) Summary
Cancellation-aware result forwarding
subscription.go, subscription_leak_test.go
The producer returns when p.Context is cancelled instead of blocking on resultChannel. The regression test covers cancellation while the producer is blocked.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 1a4d8

The change makes subscription producers stop cleanly when their context is cancelled and adds a regression test; no actionable merge-blocking risk remains beyond normal checks and review.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: fixing the producer goroutine leak when the subscription context is cancelled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 95.805% (-0.009%) from 95.814% — hsieh:master into graphql-go:master

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