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
fix: Return an error on publish if the publisher's state is not RUNNING #81
Conversation
Codecov Report
@@ Coverage Diff @@
## master #81 +/- ##
============================================
+ Coverage 60.11% 60.21% +0.09%
Complexity 388 388
============================================
Files 90 90
Lines 2046 2046
Branches 170 171 +1
============================================
+ Hits 1230 1232 +2
+ Misses 724 722 -2
Partials 92 92
Continue to review full report at Codecov.
|
Not sure if this is the best solution. |
...ublite/src/main/java/com/google/cloud/pubsublite/cloudpubsub/internal/WrappingPublisher.java
Outdated
Show resolved
Hide resolved
This should be okay. ProxyService will handle this gracefully, please add onPermanentError. |
Done. PTAL at revised implementation in PublisherImpl. |
...-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/internal/wire/PublisherImpl.java
Outdated
Show resolved
Hide resolved
@@ -236,6 +237,13 @@ void flushToStream() { | |||
} | |||
} | |||
|
|||
@VisibleForTesting |
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.
Please remove this method. You can check the permanent error handlerr to determine if a permanent error occurred.
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.
Yeah, that was the first thing I tried, but the listeners don't get called. I traced through and think it's because AbstractService::notifyFailed just throws an IllegalStateException when the state is New or Terminated, and doesn't transition to Failed. Since there's no state transition, the listeners aren't called.
I couldn't see another way to verify that PublisherImpl had permanently shutdown, so added isShutdown(). But could just omit that check in the test.
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.
Put it into STARTING or STOPPING by making the startAsync or stopAsync method of the RetryingConnection block on a CountdownLatch. Then try to publish to trigger this case, and then let the method proceed.
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, CountDownLatch is useful. I looked into your suggestion and noticed that all of our doStarts are synchronous. The only scenario that really triggers the original bug (b/156525288) is when the user publishes before calling startAsync, and so left the test to check NEW.
I removed the isShutdown() and just verified that the service has failed permanently by checking that an IllegalStateException is thrown if the user then tries to start the service. This occurs if they try to start PublisherImpl or any proxy such as WrappingPublisher.
...-cloud-pubsublite/src/main/java/com/google/cloud/pubsublite/internal/wire/PublisherImpl.java
Show resolved
Hide resolved
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.
See comments
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes internal issue b/156525288.