Skip to content
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: move await_msg_callbacks flag to subscribe() method #320

Merged
merged 6 commits into from Mar 24, 2021

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Mar 10, 2021

Reverts #315 that reverted #292.
Fixes #318.
Supersedes #319.

A full revert of #292 re-introduces several issues that have been fixed with it. This PR thus reverts the revert, but also makes the necessary (less radical) changes that avoid the error in PubSub Lite client - the StreamingPullFuture is now intact, and the await_msg_callbacks parameter of the cancel() method has been moved to subscriber client's subscribe() method.

Please test the PubSub Lite client with this PR branch to verify that it indeed doesn't break it anymore, thanks!

PR checklist:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@plamut plamut added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 10, 2021
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/python-pubsub API. label Mar 10, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Mar 10, 2021
This is to keep the StreamingPullFuture's surface intact for
compatibility with PubSub Lite client.
@plamut plamut removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 10, 2021
@plamut plamut marked this pull request as ready for review March 10, 2021 11:24
@plamut plamut requested a review from a team as a code owner March 10, 2021 11:24
@plamut plamut changed the title Revert "revert: add graceful streaming pull shutdown (#315)" fix: move await_msg_callbacks flag to subscribe() method Mar 10, 2021
@dpcollins-google
Copy link
Contributor

dpcollins-google commented Mar 10, 2021

As far as I can tell, this will not break Pub/Sub Lite python. I will separately work on making it clear which API surfaces Pub/Sub Lite overrides so it is at least clear in the future what kind of changes would break the other client, and figuring out how to test this. Implicit LGTM, but I think I have write access, so I won't officially approve.

@plamut
Copy link
Contributor Author

plamut commented Mar 10, 2021

FWIW, as per offline discussion - the cancel() itself should not block for "long", the following was proposed:

try:
   streaming_future.result()
except Something:
    streaming_future.cancel()  # should return "fast"
    streaming_future.result()  # this should block until active callbacks are done (if await_msg_callbacks was set)

Since right now the graceful shutdown is not released anywhere, we can still change the behavior. If we do, we will likely have to throw in a new sample or two to explain how the streaming pull future's result() and cancel() behave depending on the await_callbacks_on_shutdown flag.

Copy link
Contributor

@anguillanneuf anguillanneuf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Peter for the quick fix. I gave it a try and it did fix googleapis/python-pubsublite#95.

LGTM.

@plamut
Copy link
Contributor Author

plamut commented Mar 10, 2021

Good to hear that it works.

@dpcollins-google made a good point that the streaming pull future's cancel() should not block, but instead any blocking operations should be done when .result() is called.

The future.cancel() has actually been a blocking call since forever, I think the behavior dates back to Ancient Rome when threads have not been invented yet. The call blocks until the shutdown is complete, which is usually quite fast, but a graceful shutdown makes it much more noticeable.

The solution is to move the shutdown operations into their own thread, similarly to what we already do when we close the manager due to a terminal stream error. I'm probably offline tomorrow, but can add this before the end of the week.

If we are not in a hurry with the release, I'd say we wait for this improvement before merging. Otherwise just remove the label and I'll submit a separate PR.

@plamut plamut added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 10, 2021
@plamut plamut removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Mar 12, 2021
@plamut
Copy link
Contributor Author

plamut commented Mar 12, 2021

Made the streaming_pull_future.cancel() call non-blocking, the user code should block when calling future.result() after canceling it.

I will follow up with a sample, but if the PR is merged in the meantime, I can open a separate PR.

Edit: The samples README, when re-generated, should pick up the new snippet automatically, right?

@plamut plamut requested a review from a team as a code owner March 12, 2021 15:53
@plamut plamut requested review from busunkim96 and removed request for a team March 12, 2021 15:53
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 12, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 12, 2021
@plamut
Copy link
Contributor Author

plamut commented Mar 15, 2021

cla/google status update is stuck.

@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 15, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 15, 2021
@plamut plamut mentioned this pull request Mar 16, 2021
@anguillanneuf
Copy link
Contributor

anguillanneuf commented Mar 16, 2021

@plamut I force rescanned to get the CLA signed.

@snippet-bot
Copy link

snippet-bot bot commented Mar 17, 2021

Here is the summary of changes.

You are about to add 1 region tag.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@pradn
Copy link
Contributor

pradn commented Mar 19, 2021

I'll take a look at this on Monday.

@plamut plamut mentioned this pull request Mar 20, 2021
@pradn
Copy link
Contributor

pradn commented Mar 22, 2021

Apologies, got caught up today. Tomorrow for sure. :)

@plamut plamut merged commit d40d027 into googleapis:master Mar 24, 2021
@plamut plamut deleted the iss-318 branch March 24, 2021 08:37
@gnagel
Copy link

gnagel commented Mar 30, 2021

Very excited to see this fix merged in, thanks for the awesome work here :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/python-pubsub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Graceful streaming pull shutdown breaks PubSub Lite
7 participants