-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Make sure that background resources are closed when the PubSub subscriber fails #6238
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
hannahrogers-google
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.
@mdirkse, thanks for your PR! Looks good - just a small change requested for the unit test
.../google-cloud-pubsub/src/test/java/com/google/cloud/pubsub/v1/FakeFixedExecutorProvider.java
Outdated
Show resolved
Hide resolved
...oud-clients/google-cloud-pubsub/src/test/java/com/google/cloud/pubsub/v1/SubscriberTest.java
Show resolved
Hide resolved
|
@mdirkse, did you want to take a look at the requested changes so that we can merge this PR? |
|
Yes! Sorry, will do so today. |
4f40b49 to
70a0bc6
Compare
hannahrogers-google
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 @mdirkse!
Codecov Report
@@ Coverage Diff @@
## master #6238 +/- ##
============================================
+ Coverage 46% 46.36% +0.36%
- Complexity 26517 28037 +1520
============================================
Files 2614 2614
Lines 282790 288197 +5407
Branches 33598 33789 +191
============================================
+ Hits 130084 133614 +3530
- Misses 143430 144333 +903
- Partials 9276 10250 +974
Continue to review full report at Codecov.
|
|
Fixed the formatting error. |
Completes fix of #5227.
We found that the PubSub Subscriber can go into a thread creation loop when it fails on startup, eventually causing system-wide thread starvation. This PR fixes that problem by making sure that all background resources are also shut down on failure (instead of just when stopped).