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: threads can skip the line in publisher flow controller #422

Merged
merged 5 commits into from Jun 17, 2021

Conversation

plamut
Copy link
Contributor

@plamut plamut commented Jun 2, 2021

Fixes #421.

Contrary to the bytes reservations, free message slots are not distributed in FIFO order among the waiting threads, meaning that a message arriving later could be accepted before a message already waiting in the queue.

This PR fixes it by enforcing FIFO distribution of available message slots + some simplifications of the logic and data structures.

Tip: Might be easier to review commit-by-commit, each contains a well-rounded change to the codebase.

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 requested a review from a team as a code owner June 2, 2021 13:47
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/python-pubsub API. label Jun 2, 2021
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 2, 2021
@plamut plamut added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 3, 2021
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jun 3, 2021
@plamut plamut requested a review from pradn June 8, 2021 11:39
@pradn
Copy link
Contributor

pradn commented Jun 17, 2021

The solution looks good, but I worry about the increase in complexity. Do you think the risk of starvation is high enough to warrant this change?

@plamut
Copy link
Contributor Author

plamut commented Jun 17, 2021

Do you think the risk of starvation is high enough to warrant this change?

I'd say yes, here's my thinking:

  • Any reported starvation bugs could be hard to verify/reproduce consistently, thus better to get rid of this now.
  • Ruby and Java do the same, they both control free slot distribution (in FIFO order).
  • This complexity is self-contained within an independent component, making it more manageable. Besides, some of the logic has also been simplified, so the net complexity increase is not that big (IMHO). 🙂

An argument against could be that users will not even be publishing messages in multiple threads using a shared client instance, most of them will probably just publish them in a single thread in a tight loop. But since we decided back then to make the flow controller thread-safe, then let's also keep it robust, which requires some inherent complexity anyway.

@pradn
Copy link
Contributor

pradn commented Jun 17, 2021

Okay, fwiw I agree with you that it's best to prevent hard-to-debug issues now. I wish there was a standard flow control mechanism we could just pull off the shelf, because there's some subtlety in this class.

@plamut plamut merged commit ef89f55 into googleapis:master Jun 17, 2021
@plamut plamut deleted the iss-421 branch June 17, 2021 20: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 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.

A thread might skip the line in publisher flow controller
3 participants