-
Notifications
You must be signed in to change notification settings - Fork 259
mcp: fix goroutine leaks in unit tests #496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@findleyr let me know what you think when you have time :) |
findleyr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience, and for these improvements. I got busy and had to put down this review temporarily.
|
I have addressed all comments and merged the most recent |
findleyr
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just superficial comments at this point. Really appreciate your time and diligence in tracking this down.
My largest comment was that I don't want to add a dependency on go-leak, and would prefer to do this analysis in an ad-hoc manner. (I actually thought I'd already left that feedback, but alas my review was still pending--sorry).
mcp/client_example_test.go
Outdated
| // Connect the server and client... | ||
| t1, t2 := mcp.NewInMemoryTransports() | ||
| if _, err := s.Connect(ctx, t1, nil); err != nil { | ||
| sess1, err := s.Connect(ctx, t1, nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/sess1/serverSession (or ss)
s/sess2/clientSession (or cs)
sess1 and sess2 obscures the fact that these variables have different types.
go.mod
Outdated
| github.com/google/go-cmp v0.7.0 | ||
| github.com/google/jsonschema-go v0.3.0 | ||
| github.com/yosida95/uritemplate/v3 v3.0.2 | ||
| go.uber.org/goleak v1.3.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I don't think we should add an additional dependency just for this purpose. It seems like handling these as a one-off, every once in a while, is sufficient for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point and removed this from the PR so we can move ahead.
It would have been beneficial to keep an automated test because it will be very easy to introduce regressions without it, but of course that decision is up to you.
|
Hi Friedrich, we really appreciate this contribution and would like to land it. Let me know if you'd like me to take it over (mea culpa for the review latency--it has been a busy few weeks!). |
b208d65 to
c29d3a7
Compare
|
Sorry it took so long to respond. I finally took some time and addressed the remaining comments, so this should be good to merge now. If you would like to see more changes and want to move quickly, feel free to make them directly. |
|
Thanks! |
This PR fixes goroutine leaks in all unit tests.
To find the leaks I integrated https://github.com/uber-go/goleak and I suggest to keep using it to catch any future regressions.
I used the folowing script to more easily find individual tests which were leaking goroutines: