Add guard for request handling - #2475
shariarriday wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Defines and enforces request(0) as a no-op across CAF flow subscriptions.
Changes:
- Documents the zero-demand contract.
- Guards affected operators from scheduling work or changing state.
- Adds regression tests for key operators.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Records the behavior fix. |
manual/core/DataFlows.rst |
Documents zero-demand semantics. |
libcaf_core/caf/flow/subscription.hpp |
Filters zero-demand requests centrally. |
libcaf_core/caf/detail/stream_bridge.cpp |
Ignores zero bridge demand. |
libcaf_core/caf/flow/op/buffer.hpp |
Prevents zero-demand scheduling. |
libcaf_core/caf/flow/op/cache.hpp |
Prevents zero-demand updates. |
libcaf_core/caf/flow/op/cell.hpp |
Avoids registering zero-demand listeners. |
libcaf_core/caf/flow/op/cell.test.cpp |
Tests cell behavior. |
libcaf_core/caf/flow/op/from_generator.hpp |
Prevents zero-demand runs. |
libcaf_core/caf/flow/op/from_resource.hpp |
Ignores zero resource demand. |
libcaf_core/caf/flow/op/from_steps.hpp |
Ignores zero step demand. |
libcaf_core/caf/flow/op/interval.cpp |
Prevents timer arming and underflow. |
libcaf_core/caf/flow/op/interval.test.cpp |
Tests interval behavior. |
libcaf_core/caf/flow/op/mcast.test.cpp |
Tests multicast behavior. |
libcaf_core/caf/flow/op/merge.hpp |
Guards merge requests. |
libcaf_core/caf/flow/op/prefix_and_tail.hpp |
Guards prefix demand. |
libcaf_core/caf/flow/op/ucast.hpp |
Guards unicast demand. |
libcaf_core/caf/flow/op/ucast.test.cpp |
Tests unicast behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2475 +/- ##
==========================================
- Coverage 73.30% 73.14% -0.17%
==========================================
Files 645 657 +12
Lines 30836 31433 +597
Branches 3385 3446 +61
==========================================
+ Hits 22605 22992 +387
- Misses 6319 6480 +161
- Partials 1912 1961 +49 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d223eae to
fe7615f
Compare
fe7615f to
a3f75a8
Compare
| void request(size_t n) { | ||
| pimpl_->request(n); | ||
| if (n > 0) | ||
| pimpl_->request(n); |
There was a problem hiding this comment.
I don't think we need any other check besides this one, or can you find a counter-example (outside of tests) where we hold a pointer directly instead of going through a subscription? As it stands, I think the checks inside the operators are purely redundant. Maybe we can put a CAF_ASSERT into the operators.
| /// Signals demand for `n` more items. Calling this member function with | ||
| /// `n == 0` is a no-op: it neither adds demand nor triggers any observable | ||
| /// side effect such as arming a timer or scheduling work. |
There was a problem hiding this comment.
| /// Signals demand for `n` more items. Calling this member function with | |
| /// `n == 0` is a no-op: it neither adds demand nor triggers any observable | |
| /// side effect such as arming a timer or scheduling work. | |
| /// Signals demand for `n` more items. | |
| /// @pre `n > 0` |
We enforce this in the request on the subscription object. Hence, we don't need to re-check in every single implementation (except maybe putting a CAF_ASSERT).
| } | ||
| } | ||
|
|
||
| SCENARIO("requesting zero items from a cell is a no-op") { |
There was a problem hiding this comment.
Please think about what you are testing here. There's an assert in the cell for n > 0. The only reason this doesn't crash is because subscription will discard any request with n == 0 and not forward it to the implementation. This test doesn't add any useful coverage. The other new tests probably as well. Please make a pass over the PR to see what's actually worth keeping. Then rebase and squash.
There was a problem hiding this comment.
Yes, there are still some pending issues and I did not re-request the PR review for that reason. I am converting the PR to draft for now to avoid confusion.
Relates #2471