Fix goroutine leaks in grpc integration tests #5340
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5340 +/- ##
==========================================
+ Coverage 95.18% 95.21% +0.03%
==========================================
Files 343 343
Lines 16780 16787 +7
==========================================
+ Hits 15972 15984 +12
+ Misses 608 605 -3
+ Partials 200 198 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
WillSewell
force-pushed
the
fix-goroutine-leaks-in-grpc-integration-tests
branch
from
643e366
to
e1db93f
Compare
The key issue was that the grpc client was only being killed in a `runtime.SetFinalizer` - i.e. when it is GCed. I think in the tests this was not shutting down before goleak had decided that the goroutine had leaked. This change switches to a more conventional approach of killing the client as part of the Close method and then ensuring this is called in each of the tests. While this change does not include a call to active goleak, it was tested with this. It does not include goleak detection because there are still other violations related to elasticsearch in this package. Signed-off-by: Will Sewell <[email protected]>
WillSewell
force-pushed
the
fix-goroutine-leaks-in-grpc-integration-tests
branch
from
e1db93f
to
74d4c6e
Compare
yurishkuro
added
the
changelog:test
|
I merged a large PR that affected some of the test files. There are now merge conflicts (which would've been more difficult to resolve in that large PR than here, so I picked this one to resolve conflicts). |
Signed-off-by: Will Sewell <[email protected]>
|
No worries. The changes actually make this PR simpler. One area that is pretty unpleasant about this change is the fact that that each subtest is calling I think a nicer approach would be to introduce separate a |
|
I also don't like how CleanUp functions often call initialize(), this was not at all how it was meant to work, the cleanup functions were intended to remove data from the DB, not to reset the connection completely. I am not opposed to larger refactoring (may not be as large as it seems) if we can make the workflow more robust and more intuitive. I don't even remember why Refresh callback was needed - nothing implements it except for ES, and in case of ES the reason seems to be to ensure that the data is queryable after write once Refresh returns, which actually doesn't seem to work reliably anyway and we already have most of the assertions on reading data executing within waitForCondition, which makes synchronous flushing irrelevant. |
| @@ -109,6 +113,7 @@ func TestGRPCStorage(t *testing.T) { | |||
| } | |||
| s.initialize(t) | |||
| s.RunAll(t) | |||
| s.close(t) | |||
There was a problem hiding this comment.
nit: should really be called before RunAll via defer()
Signed-off-by: Yuri Shkuro <[email protected]>
## Which problem is this PR solving? - Solves part of jaegertracing#5006 ## Description of the changes - The key issue was that the grpc client was only being killed in a `runtime.SetFinalizer` - i.e. when it is GCed. I think in the tests this was not shutting down before goleak had decided that the goroutine had leaked. - This change switches to a more conventional approach of killing the client as part of the Close method and then ensuring this is called in each of the tests. ## How was this change tested? - While this change does not include a call to activate goleak, it _was_ tested with this. The PR does not include goleak detection because there are still other violations related to elasticsearch in this package. ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [ ] I have added unit tests for the new functionality - [ ] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Will Sewell <[email protected]> Signed-off-by: Yuri Shkuro <[email protected]> Co-authored-by: Yuri Shkuro <[email protected]> Signed-off-by: Vamshi Maskuri <[email protected]>
Which problem is this PR solving?
Description of the changes
runtime.SetFinalizer- i.e. when it is GCed. I think in the tests this was not shutting down before goleak had decided that the goroutine had leaked.How was this change tested?
Checklist
jaeger:make lint testjaeger-ui:yarn lintandyarn test