Draft
Conversation
This change introduces a `done` channel to gracefully shut down context monitoring goroutines when function calls or streams complete, preventing potential deadlocks. Co-authored-by: aaron <aaron@boundaryml.com>
|
Cursor Agent can help with this pull request. Just |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
| for res := range callback { | ||
| wrappedCallback <- res | ||
| } | ||
| }() |
There was a problem hiding this comment.
Wrapper goroutine can leak on context cancellation
The wrapper goroutine in CallFunctionStream is not context-aware. When context is cancelled, if the consumer stops reading from wrappedCallback (a common pattern), the goroutine blocks forever on wrappedCallback <- res. The PR fixes one goroutine leak but introduces a new one. The send to wrappedCallback needs to use a select that also checks ctx.Done() to allow the goroutine to exit when context is cancelled.
This change passes the context object to BAML client functions, which is necessary for proper tracing and cancellation. It also adds a WithClient option to the call options, allowing for more flexible client configuration. Co-authored-by: aaron <aaron@boundaryml.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Template
Thanks for taking the time to fill out this pull request!
Issue Reference
Please link to any related issues
CallFunction...BAML runtime methods leak goroutines #2883Changes
Please describe the changes proposed in this pull request
This PR addresses a goroutine leak in the Go BAML client when
context.Background()is used for function calls. Previously, a goroutine was spawned to monitorctx.Done()for cancellation. Ifcontext.Background()was passed, this context would never complete, causing the monitoring goroutine to leak indefinitely even after the function call finished.The fix introduces a
donechannel forCallFunction,CallFunctionStream, andCallFunctionParse. This channel is closed when the function or stream completes normally. The monitoring goroutine now uses aselectstatement to listen on bothctx.Done()and the newdonechannel, ensuring it exits cleanly upon either cancellation or normal function completion.Testing
Please describe how you tested these changes
go build ./...)go test ./...)Screenshots
If applicable, add screenshots to help explain your changes
[Add screenshots here...]
PR Checklist
Please ensure you've completed these items
Additional Notes
Add any other context about the PR here
The core problem was that
context.Background()never signalsDone(), leading to leaked goroutines. Thedonechannel provides an alternative signal for normal completion, ensuring proper resource cleanup.Slack Thread
Note
Fix goroutine leak in Go runtime
donechannels toCallFunction,CallFunctionStream, andCallFunctionParseso the context-monitor goroutines exit on normal completion orctx.Done()CallFunctionStream, closedoneon error/result and wrap the callback channel to ensuredoneis closed when the stream endsWritten by Cursor Bugbot for commit d0f1f50. This will update automatically on new commits. Configure here.