Repository navigation
Conversation
|
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@beam.apache.org list. Thank you for your contributions. |
|
This pull request has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time. |
4bb3e96 to
cf16a90
Compare
|
Converting it back to draft since some tests are failing. |
…Service
Prevent leaked gRPC ManagedChannel errors ("Previous channel ... was
garbage collected without being shut down!") in the Java SDK harness:
- Shut down the provision ManagedChannel in ExternalWorkerService.startWorker
after fetching ProvisionInfo.
- Retain created ManagedChannels in BeamFnDataGrpcClient and shut them
down when BeamFnDataGrpcClient.close() is called.
- Shut down state ManagedChannels in BeamFnStateGrpcClientCache on close.
- Close BeamFnDataGrpcClient, BeamFnStateGrpcClientCache, and the control
ManagedChannel during FnHarness shutdown.
cf16a90 to
98687da
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39663 +/- ##
============================================
+ Coverage 56.16% 65.05% +8.88%
- Complexity 2288 13977 +11689
============================================
Files 1124 1729 +605
Lines 178336 108487 -69849
Branches 1489 11307 +9818
============================================
- Hits 100170 70571 -29599
+ Misses 75645 33707 -41938
- Partials 2521 4209 +1688
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
added
Next Action: Reviewers
and removed
Next Action: Author
stale
labels
| beamFnStatusClient.close(); | ||
| } | ||
| processBundleHandler.shutdown(); | ||
| beamFnDataMultiplexer.close(); |
There was a problem hiding this comment.
processBundleHandler.shutdown() already calls beamFnStateGrpcClientCache.close(); and beamFnDataClient.close();
This is redundant though probably harmless, however we should close client in the place where it owns it in general, to avoid potential pre-mature channel break
|
Assigning reviewers: R: @Abacn for label java. This pull request likely touches a core component ("core" label). Please review with scrutiny. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
2 participants
Description
Follow-up to #39390.
While #39390 added
BeamFnDataGrpcClient.close()to tear down activeBeamFnDataGrpcMultiplexerstream observers onProcessBundleHandlershutdown, the underlying gRPCManagedChannels created in the Java SDK harness were still not retained or shut down. When garbage collection runs (for example, in loopback workers started viaExternalWorkerService), gRPC'sManagedChannelOrphanWrapperlogsERROR/SEVEREmessages:Changes
ExternalWorkerService: Store theManagedChannelcreated forProvisionEndpointinstartWorkerand callchannel.shutdown()in afinallyblock after fetchingProvisionInfo.BeamFnDataGrpcClient: Retain createdManagedChannelinstances inchannelCache(preventingManagedChannelOrphanWrapperfrom being garbage-collected while streams are active) and shut them down inBeamFnDataGrpcClient.close().BeamFnStateGrpcClientCache: Shut down the stateManagedChannelwhen aGrpcStateClientcloses and addBeamFnStateGrpcClientCache.close().FnHarness: CloseBeamFnDataGrpcClient,BeamFnStateGrpcClientCache, and shut down the controlManagedChannelduring harness shutdown.Code to reproduce
Error messages (two leaking channels)
With the fix in this PR, the above error messages are gone.