Skip to content

fix(pubsub/v2): acquire bounded limits in flow controller - #12590

Merged
hongalex merged 5 commits into
googleapis:mainfrom
hongalex:fix-pubsub-fc-count
Jul 29, 2025
Merged

hongalex merged 5 commits into
googleapis:mainfrom
hongalex:fix-pubsub-fc-count

Conversation

@hongalex

@hongalex hongalex commented Jul 18, 2025 •

Copy link
Copy Markdown
Member

This change makes flow controller continue to track and record outstanding messages/bytes even when flow control mechanisms are disabled.

Fixes #12447

@hongalex
hongalex requested a review from shollyman as a code owner July 18, 2025 18:43
@hongalex
hongalex requested review from a team July 18, 2025 18:43
@product-auto-label product-auto-label Bot added the api: pubsub Issues related to the Pub/Sub API. label Jul 18, 2025
bhshkh
bhshkh previously approved these changes Jul 28, 2025
f.recordOutstandingBytes(ctx, outstandingBytes)
f.semSize.Release(f.bound(size))
if f.limitBehavior != FlowControlIgnore {
f.semSize.Release(f.bound(size))

@bhshkh bhshkh Jul 28, 2025 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

A semaphore requires that we release the exact same number of permits that we acquired.

The acquire function has different logic for different LimitExceededBehavior values:
- In FlowControlBlock mode, it acquires f.bound(size) from the semaphore.
- In FlowControlSignalError mode, it attempts to acquire the raw int64(size).

The release function always releases f.bound(size). It incorrectly assumes that the amount acquired was also f.bound(size). Could this manifest into a bug?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch, this has actually been out for a while. Most likely, it's because the bounding behavior doesn't trigger often (unless users set flow control bytes to very low).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So actually I remember why this was intentional.

In FlowControlSignalError mode, the acquire never goes through since the flow controller throws an error if there is not enough space for the message. In such a case, we bubble up the error ErrFlowControllerMaxOutstandingBytes. We shouldn't bound the message size here, since we consider "too large messages" an error.

@hongalex hongalex changed the title fix(pubsub): update flowcontrol metrics even when disabled fix(pubsub): acquire bounded limits in flow controller Jul 29, 2025
bhshkh
bhshkh previously approved these changes Jul 29, 2025
@hongalex hongalex changed the title fix(pubsub): acquire bounded limits in flow controller fix(pubsub/v2): acquire bounded limits in flow controller Jul 29, 2025
This reverts commit e816d1e5e9aucd9b07665ce5fe80ecf7a8c3b950.
@hongalex
hongalex merged commit c153495 into googleapis:main Jul 29, 2025
@hongalex
hongalex deleted the fix-pubsub-fc-count branch July 29, 2025 21:40
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pubsub: outstanding bytes and outstanding messages metrics are 0 when FlowControlIgnore is set

2 participants