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)
@pradn
Copy link
Contributor

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

Loading

@plamut
Copy link
Contributor Author

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

Loading

@pradn
Copy link
Contributor

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

Loading

pradn
pradn approved these changes Jun 17, 2021
@plamut plamut merged commit ef89f55 into googleapis:master Jun 17, 2021
9 checks passed
Loading
@plamut plamut deleted the iss-421 branch Jun 17, 2021
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.

3 participants