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

PubSub: Release the state lock before calling the publish api #8234

Merged
merged 2 commits into from
Jun 13, 2019

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Jun 6, 2019

Closes #8036.

Supersedes #7686 (the fix here is essentially the same, but also adds tests and a few minor things).

Summary

This PR makes sure that calls to publisher.publish() are not unnecessarily blocked while the underlying API publish call is in progress - the latter can take a substantial amount of time, especially on network errors when retries kick in.

Description

The fix makes sure that the lock in the _commit() method is held for no longer than necessary. The details on why releasing the lock earlier is fine are listed in the comment on the original PR.

How to test

Steps to reproduce:

  • Set the total publish timeout setting to 60,000 milliseconds (in order to not wait for too long),
  • Disable the internet connection on your machine,
  • Run the test publisher script:
import logging

from google.cloud import pubsub_v1


log_format = (
    "%(levelname)-8s [%(asctime)s] %(threadName)-33s "
    "[%(name)s] [%(filename)s:%(lineno)d][%(funcName)s] %(message)s"
)
logging.basicConfig(
    level=logging.DEBUG,
    format=log_format,
)
logger = logging.getLogger(__name__)

PROJECT_NAME = "some-pubsub-project"
TOPIC_NAME = "my-topic"


def main():
    publisher = pubsub_v1.PublisherClient()
    topic_path = publisher.topic_path(PROJECT_NAME, TOPIC_NAME)

    message_future = publisher.publish(topic_path, b"I'm going to timeout")

    try:
        message_future.exception(timeout=3)
    except Exception as exc:
        logger.info("\x1b[1mException raised:\x1b[0m {}".format(exc))

    logger.info("\x1b[1mAbout to publish the second message\x1b[0m")
    publisher.publish(topic_path, b"I'm going to take quite some time to return")
    logger.info("\x1b[1mReturned from second call to publish()\x1b[0m")


if __name__ == "__main__":
    main()

Actual result (before the fix):
The message "Returned from second call to publish" appears in the logs only after a minute or so after the "About to publish the second message".

Expected result (after the fix):
The second message is logged shortly after the first one. The second call to publisher.publish() is not blocked while publishing the first message is in progress and hitting the automatic retry because of the network error.

Once the publish batch transitions to IN_PROGRESS state, any subsequent
calls to commit the batch effectively become a no-op. The state lock
can thus be released immediately after the state change, unblocking
other threads that might be waiting to publish another PubSub  message.

Co-authored by @sayap (GitHub) and Rencana Tarigan [email protected]
googleapis#7686
@plamut plamut added the api: pubsub Issues related to the Pub/Sub API. label Jun 6, 2019
@plamut plamut requested a review from busunkim96 as a code owner June 6, 2019 12:12
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 6, 2019
@sduskis sduskis requested a review from tswast June 6, 2019 17:32
@plamut plamut requested a review from anguillanneuf as a code owner June 11, 2019 09:36
@plamut plamut requested a review from tswast June 11, 2019 09:37
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 11, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 11, 2019
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 11, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 11, 2019
@plamut plamut merged commit f3162f7 into googleapis:master Jun 13, 2019
@plamut plamut deleted the iss-8036 branch June 18, 2019 06:55
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 Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pub/Sub: publish message hangs waiting for previous publish to timeout
4 participants