Skip to content

Conversation

@anuraaga
Copy link
Contributor

Commit Message: dynamic_modules: trigger downstream watermark callbacks when terminal

Additional Description:

Currently, dynamic modules have no way of knowing when the downstream buffers are filling to be able to apply response backpressure. This is notably important for terminal filters that actually generate the response.

On the flip side, non-terminal filters shouldn't need it. I considered making filters opt-in to the callbacks but felt the terminal filter knob may be enough of a signal and kept it simple. This comes from reading the flow control doc and its notion that the router filter opts into notifications to avoid "number of streams * number of filters" callbacks - I think terminal dynamic modules are the equivalent of router in this context. Let me know any thoughts.

the downstream connection. It passes events to the router filter via

Risk Level: Low
Testing: Integration test
Docs Changes:
Release Notes: N/A
Platform Specific Features: N/A
Fixes #41432

/cc @mathetake @wbpcode

Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
FilterMetadataStatus decodeMetadata(MetadataMap&) override;
void setDecoderFilterCallbacks(StreamDecoderFilterCallbacks& callbacks) override {
decoder_callbacks_ = &callbacks;
if (config_ && config_->terminal_filter_) {
Copy link
Contributor Author

@anuraaga anuraaga Oct 14, 2025

Choose a reason for hiding this comment

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

config_ can only be nullptr in some implementation detail unit tests and instead of making tests pass the expected config I just added the null check. Maybe my approach of registering the watermark callbacks implicitly like this isn't good after all, let me know

Signed-off-by: Anuraag Agrawal <[email protected]>
@mathetake mathetake self-assigned this Oct 14, 2025
@mathetake
Copy link
Member

@anuraaga super busy so let me get back to you next week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Response backpressure for dynamic modules

2 participants