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)
This is to keep the StreamingPullFuture's surface intact for
compatibility with PubSub Lite client.
@plamut plamut marked this pull request as ready for review Mar 10, 2021
@plamut plamut requested a review from as a code owner Mar 10, 2021
@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 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.

Loading

@plamut
Copy link
Contributor Author

@plamut 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.

Loading

Copy link
Contributor

@anguillanneuf anguillanneuf left a comment

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

LGTM.

Loading

@plamut
Copy link
Contributor Author

@plamut 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.

Loading

@plamut plamut requested a review from anguillanneuf Mar 12, 2021
@plamut
Copy link
Contributor Author

@plamut 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?

Loading

@plamut plamut requested a review from as a code owner Mar 12, 2021
@plamut plamut requested review from busunkim96 and removed request for Mar 12, 2021
@plamut
Copy link
Contributor Author

@plamut plamut commented Mar 15, 2021

cla/google status update is stuck.

Loading

@anguillanneuf
Copy link
Contributor

@anguillanneuf anguillanneuf commented Mar 16, 2021

@plamut I force rescanned to get the CLA signed.

Loading

@snippet-bot
Copy link

@snippet-bot 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

Loading

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

@pradn pradn commented Mar 19, 2021

I'll take a look at this on Monday.

Loading

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

@pradn pradn commented Mar 22, 2021

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

Loading

pradn
pradn approved these changes Mar 23, 2021
@plamut plamut merged commit d40d027 into googleapis:master Mar 24, 2021
8 checks passed
Loading
@plamut plamut deleted the iss-318 branch Mar 24, 2021
@gnagel
Copy link

@gnagel gnagel commented Mar 30, 2021

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants