Skip to content

test(facade): cover shared RESP V2 buffer - #8180

Open
glevkovich wants to merge 2 commits into
mainfrom
glevkovich/shared_proactor_buffer_add_tests_pr4
Open

test(facade): cover shared RESP V2 buffer#8180
glevkovich wants to merge 2 commits into
mainfrom
glevkovich/shared_proactor_buffer_add_tests_pr4

Conversation

@glevkovich

@glevkovich glevkovich commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

This is a follow-up test coverage to feat(facade): share the RESP V2 read buffer.

Note:

  • Shared buffer is a base (shim) to provided buffer, and can also be used in production kernels under 6.12 and when using epoll.
  • ASan, UBSAn and fuzzing are not part of this commit.

GoogleTest:

  1. Add GoogleTest coverage in proactor_read_buffer_test.cc for exclusive borrowing, empty-buffer release, and preventing borrows from crossing a fiber switch.
  2. Extend resp_srv_parser_test.cc with source-reuse and fixed-seed fragmentation coverage for inline, multibulk, and large bulk requests.

Regression Tests/pytest:
Add integration coverage for interleaved clients, pipeline backpressure and overflow recovery, blocking commands, migration, protocol errors, RESET/PAUSE, DEBUG TRAFFIC, TLS, Unix/admin listeners, accounting, and io_uring provided-buffer exclusion.

Use explicit V1/V2 and shared-buffer settings so private and shared buffer behavior are tested independently.

@glevkovich
glevkovich requested review from romange and a balanced review from Copilot August 27, 2026 12:11
@qodo-free-for-open-source-projects

qodo-free-for-open-source-projects Bot commented Aug 27, 2026

Copy link
Copy Markdown

PR Summary by Qodo

Expand shared RESP V2 buffer regression coverage

🧪 Tests 🕐 40+ Minutes


AI Description

• Tests exclusive shared-buffer borrowing, empty release, and same-fiber ownership invariants.
• Verifies parser correctness under source reuse, interleaving, and deterministic fragmentation.
• Covers shared RESP V2 concurrency, backpressure, migration, TLS, listeners, metrics, and io_uring.
Diagram

sequenceDiagram
  actor A as Client A
  actor B as Client B
  participant Loop as RESP V2 Loop
  participant Shared as Shared Buffer
  participant Parser as RESP Parser
  participant Overflow as Overflow Copy
  participant Queue as Command Queue
  A->>Loop: Fragmented request
  Loop->>Shared: Borrow exclusively
  Shared->>Parser: Parse bytes
  Parser-->>Loop: Owned parse state
  alt Pipeline backpressure
    Loop->>Overflow: Preserve suffix
    Overflow-->>Loop: Restore after wake
  end
  Loop->>Shared: Release empty
  B->>Loop: Interleaved request
  Loop->>Shared: Borrow next
  Shared->>Parser: Parse safely
  Loop->>Queue: Dispatch commands
  Queue-->>A: Ordered replies
  Queue-->>B: Ordered replies
Loading
High-Level Assessment

The layered strategy is appropriate: focused C++ tests pin ownership and parser invariants, while end-to-end tests exercise production connection paths and backend-specific exclusions. A more parameterized shared fixture could reduce repeated server arguments, but explicit per-scenario configuration better isolates shared versus private buffer behavior and preserves clear failure diagnostics.

Files changed (4) +894 / -8

Other (4) +894 / -8
CMakeLists.txtRegister the proactor read-buffer unit test +1/-0

Register the proactor read-buffer unit test

• Adds the new GoogleTest binary to the facade test suite and links it against the facade library.

src/facade/CMakeLists.txt

proactor_read_buffer_test.ccTest shared-buffer borrowing invariants +61/-0

Test shared-buffer borrowing invariants

• Adds focused tests for exclusive ownership and successful reuse after release. Debug death tests enforce empty-buffer release and prohibit retaining a borrow across fiber switches.

src/facade/proactor_read_buffer_test.cc

resp_srv_parser_test.ccExercise parser state across reusable input buffers +124/-0

Exercise parser state across reusable input buffers

• Adds source-overwrite, complete-fragment consumption, large-bulk assembly, and interleaved parser tests. A fixed-seed fragmentation test covers both inline and multibulk commands reproducibly.

src/facade/resp_srv_parser_test.cc

connection_test.pyAdd end-to-end shared RESP V2 buffer regressions +708/-8

Add end-to-end shared RESP V2 buffer regressions

• Adds integration coverage for interleaved clients, fragmentation, pipelines, overflow recovery, blocking and control commands, migration, protocol errors, TLS, Unix/admin listeners, accounting, DEBUG TRAFFIC, and io_uring exclusion. Existing backpressure and TLS tests now explicitly distinguish V1, private V2, and shared-buffer V2 configurations.

tests/dragonfly/connection_test.py

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review
Tip of the day
💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗


Powered by Qodo

@qodo-code-review

This comment was marked as resolved.

Copilot AI left a comment

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.

Pull request overview

Adds coverage for shared RESP V2 read-buffer correctness across parser, buffer ownership, and integration scenarios.

Changes:

  • Tests exclusive borrowing and fiber-switch invariants.
  • Adds parser fragmentation and source-reuse tests.
  • Expands integration coverage across clients, backpressure, TLS, migration, and listeners.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
tests/dragonfly/connection_test.py Adds shared-buffer integration and configuration tests.
src/facade/resp_srv_parser_test.cc Tests fragmented parsing and source-buffer reuse.
src/facade/proactor_read_buffer_test.cc Tests shared-buffer borrowing invariants.
src/facade/CMakeLists.txt Registers the new unit-test target.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread tests/dragonfly/connection_test.py
@augmentcode

augmentcode Bot commented Aug 27, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR expands regression coverage for the RESP V2 shared read buffer.

Changes:

  • Adds a dedicated C++ target for ProactorReadBuffer ownership and debug-invariant tests.
  • Adds parser tests for source-buffer reuse, fragmented bulk requests, and seeded fragmentation.
  • Adds integration tests for interleaved clients, pipeline overflow/backpressure, blocking commands, migration, protocol errors, and control commands.
  • Covers TLS, Unix/admin listeners, buffer accounting, DEBUG TRAFFIC, and io_uring provided-buffer exclusion.
  • Updates existing V2 backpressure and TLS pipeline matrices to test private and shared configurations separately.

Technical Notes: Tests use small shared buffers and explicit proactor counts to force reuse, overflow copying, and cross-connection behavior.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread tests/dragonfly/connection_test.py Outdated
Follow-up to feat(facade): share the RESP V2 read buffer.

GoogleTest:
1) Add GoogleTest coverage in proactor_read_buffer_test.cc for exclusive
borrowing, empty-buffer release, and preventing borrows from crossing a
fiber switch.
2) Extend resp_srv_parser_test.cc with source-reuse and fixed-seed
fragmentation coverage for inline, multibulk, and large bulk requests.

Regression Tests/pytest:
Add integration coverage for interleaved clients, pipeline backpressure
and overflow recovery, blocking commands, migration, protocol errors,
RESET/PAUSE, DEBUG TRAFFIC, TLS, Unix/admin listeners, accounting, and
io_uring provided-buffer exclusion.

Use explicit V1/V2 and shared-buffer settings so private and shared
buffer behavior are tested independently.
@glevkovich
glevkovich force-pushed the glevkovich/shared_proactor_buffer_add_tests_pr4 branch from 7a2e968 to d4d4d11 Compare August 27, 2026 12:53
Signed-off-by: Gil Levkovich <69595609+glevkovich@users.noreply.github.com>
@kostasrim

kostasrim commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

I am slightly confused with the direction and maybe I missed something from thursday/friday or in general 😄

Have we decided that we will use the shared read buffer or are we replacing it with provided buffers ? Last time I wrote these comments #8145 (comment) mentioning that the shared read buffer is workaround to the provided buffers.

Are we following with both solutions, one or ? (I am asking so I know what to comment on and what not)

@glevkovich

Copy link
Copy Markdown
Contributor Author

I am slightly confused with the direction and maybe I missed something from thursday/friday or in general 😄

Have we decided that we will use the shared read buffer or are we replacing it with provided buffers ? Last time I wrote these comments #8145 (comment) mentioning that the shared read buffer is workaround to the provided buffers.

Are we following with both solutions, one or ? (I am asking so I know what to comment on and what not)

@kostasrim
your confusion make sense since I did not clarify things correctly, and did not have all information when we spoke. The direction is to keep both mechanisms because they solve different parts of the receive path and do not conflict.

The shared proactor read buffer is not being replaced by provided buffers. It remains the solution for cases where provided buffers cannot or should not be used:

  • Pure epoll proactor.
  • TLS connections: provided-buffer receives do not work with the current TLS path. Supporting that would require a separate kTLS-oriented design which is a large work by itself, and not planned for anytime soon.
  • io_uring instances that do not support the required incremental provided-buffer capability (old kernels).
  • Runtime fallback when a provided-buffer ring is exhausted or a connection must leave the provided-buffer path temporarily.

The provided-buffer work is an io_uring only optimization for non-TLS connections. The shared-buffer work is still required independently.In addition, It does help with io-uring development since it also establishes the receive/parse invariant for the provided-buffer iouring work: acquire a buffer (no need to read in this case), parse it without preemption. Provided buffers have a stricter lifetime rules, but the parsing and staging rules are closely related.

So my idea is to keep and test the shared proactor buffer as the general pull backend + Keep the shared buffer as the automatic fallback, not as a competing implementation.

This PR is therefore still useful since it provides coverage for the shared pull backend, but it also provides the regression baseline for the later provided-buffer implementation.

@kostasrim

Copy link
Copy Markdown
Contributor

So my idea is to keep and test the shared proactor buffer as the general pull backend + Keep the shared buffer as the

Sure no issue with that. Approved.

The shared proactor read buffer is not being replaced by provided buffers. It remains the solution for cases where provided buffers cannot or should not be used:

I am not sure why we need to worry about any of these when we have not yet proved that ioloop v2 is actually better than v1 🤷

@glevkovich

Copy link
Copy Markdown
Contributor Author

So my idea is to keep and test the shared proactor buffer as the general pull backend + Keep the shared buffer as the

Sure no issue with that. Approved.

The shared proactor read buffer is not being replaced by provided buffers. It remains the solution for cases where provided buffers cannot or should not be used:

I am not sure why we need to worry about any of these when we have not yet proved that ioloop v2 is actually better than v1 🤷

@kostasrim I agree. That's why I'm holding with merging this PR and not merging it for now.

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.

3 participants