mcp: fix goroutine leaks in unit tests#496
mcp: fix goroutine leaks in unit tests#496findleyr merged 16 commits intomodelcontextprotocol:mainfrom
Conversation
fgrosse
changed the title
|
@findleyr let me know what you think when you have time :) |
findleyr
left a comment
findleyr
left a comment
There was a problem hiding this comment.
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
findleyr
left a comment
There was a problem hiding this comment.
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.
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.
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.
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!). |
fgrosse
force-pushed
the
fgrosse/goleaks
branch
from
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! |
2 participants
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: