Skip to content

THRIFT-5830: Add per-call transport support for netstd - #4000

Open
birschick-bq wants to merge 1 commit into
apache:masterfrom
birschick-bq:dev/birschick-bq/v3/thrift-5830
Open

birschick-bq wants to merge 1 commit into
apache:masterfrom
birschick-bq:dev/birschick-bq/v3/thrift-5830

Conversation

@birschick-bq

@birschick-bq birschick-bq commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

THRIFT-5830: Add per-call transport support for netstd
Client: cpp,netstd

Add opt-in per-call transports for generated asynchronous clients, isolating HTTP and layered transport state while preserving existing shared-protocol constructors.

Preserve caller-configured HTTP timeouts and propagate the linked cancellation token through generated request and response operations. Add focused transport, lifecycle, concurrency, timeout, generator, and tutorial coverage.

Co-Authored-By: GitHub Copilot

Note: AI/OpenSpec artifacts are completely optional. Let me know if you want to keep these or have me remove them.

  • Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@birschick-bq
birschick-bq requested a review from Jens-G as a code owner October 6, 2026 20:23
Copilot AI balanced review requested due to automatic review settings October 6, 2026 20:24
@mergeable mergeable Bot added c# Pull requests that update C# code Pull requests that update .NET code compiler github_actions Pull requests that update GitHub Actions code labels Oct 6, 2026

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.

Copilot review overview

🟡 Changes recommended

Wrapper capability detection, HTTP option preservation, and undiscoverable GitHub configuration introduce functional gaps.

Review effort: Balanced
Findings: 4 High severity · 3 Medium severity · 3 Low severity

Open (10)
What changed in this PR

Adds opt-in per-call transport isolation for concurrent asynchronous netstd RPC clients.

Changes:

  • Adds per-call transport APIs, generated constructors, cancellation, timeout, and lifecycle handling.
  • Adds concurrency and transport tests plus tutorial coverage.
  • Adds documentation and OpenSpec/Copilot configuration.
File Description
tutorial/​netstd/​smoketest.sh Exercises per-call tutorial mode.
tutorial/​netstd/​README.md Documents tutorial usage.
tutorial/​netstd/​Client/​Program.cs Implements concurrent per-call example.
lib/​netstd/​Thrift/​Transport/​Layered/​TFramedTransport.cs Delegates per-call framed transports.
lib/​netstd/​Thrift/​Transport/​Layered/​TBufferedTransport.cs Delegates per-call buffered transports.
lib/​netstd/​Thrift/​Transport/​ITPerCallTransportProvider.cs Defines the provider contract.
lib/​netstd/​Thrift/​Transport/​Client/​THttpTransport.cs Adds isolated HTTP state and leasing.
lib/​netstd/​Thrift/​TBaseClient.cs Manages call-scoped protocols and deadlines.
lib/​netstd/​Tests/​Thrift.Tests/​Transports/​THttpTransportTests.cs Tests supplied HTTP timeouts.
lib/​netstd/​Tests/​Thrift.Tests/​Transports/​THttpTransportPerCallTests.cs Tests HTTP concurrency and lifecycle.
lib/​netstd/​Tests/​Thrift.Tests/​Transports/​TBaseClientPerCallTests.cs Tests client and wrapper behavior.
lib/​netstd/​README.md Documents the public feature.
lib/​netstd/​per-call-transport.md Records feature requirements.
lib/​netstd/​openspec/​specs/​per-call-transport/​spec.md Defines capability scenarios.
lib/​netstd/​openspec/​specs/​.gitkeep Retains the specs directory.
lib/​netstd/​openspec/​config.yaml Configures OpenSpec.
lib/​netstd/​openspec/​changes/​archive/​2026-10-06-per-call-transport/​tasks.md Archives implementation tasks.
lib/​netstd/​openspec/​changes/​archive/​2026-10-06-per-call-transport/​specs/​per-call-transport/​spec.md Archives the specification delta.
lib/​netstd/​openspec/​changes/​archive/​2026-10-06-per-call-transport/​proposal.md Archives the proposal.
lib/​netstd/​openspec/​changes/​archive/​2026-10-06-per-call-transport/​design.md Archives design decisions.
lib/​netstd/​openspec/​changes/​archive/​2026-10-06-per-call-transport/​.openspec.yaml Records archive metadata.
lib/​netstd/​openspec/​changes/​archive/​.gitkeep Retains the archive directory.
lib/​netstd/​.github/​workflows/​copilot-setup-steps.yml Adds Copilot setup automation.
lib/​netstd/​.github/​agents/​openspec.agent.md Defines an OpenSpec agent.
compiler/​cpp/​src/​thrift/​generate/​t_netstd_generator.cc Generates per-call client APIs.

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

Comment thread .github/agents/openspec.agent.md
Comment thread .github/workflows/copilot-setup-steps.yml
Comment thread lib/netstd/Thrift/Transport/Layered/TBufferedTransport.cs
Comment thread lib/netstd/Thrift/Transport/Layered/TFramedTransport.cs
Comment thread lib/netstd/.github/workflows/copilot-setup-steps.yml Outdated
Comment thread lib/netstd/Thrift/Transport/Client/THttpTransport.cs Outdated
Comment thread lib/netstd/per-call-transport.md
Comment thread lib/netstd/.github/workflows/copilot-setup-steps.yml Outdated
Comment thread lib/netstd/per-call-transport.md Outdated
Comment thread tutorial/netstd/README.md Outdated
Copilot AI balanced review requested due to automatic review settings October 6, 2026 21:04
@birschick-bq
birschick-bq force-pushed the dev/birschick-bq/v3/thrift-5830 branch from 1efff86 to a510284 Compare October 6, 2026 21:04
@mergeable mergeable Bot added the build and general CI cmake, automake and build system changes label Oct 6, 2026

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.

Comment thread lib/netstd/Makefile.am
Comment thread lib/netstd/Thrift/Transport/Layered/TBufferedTransport.cs Outdated
Comment thread lib/netstd/Thrift/Transport/Layered/TFramedTransport.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 6, 2026 21:25

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.

Copilot review overview

🟡 Changes recommended

Buffered-wrapper construction can leak its call-owned transport, and unrelated recurring OpenSpec CI infrastructure should be separated.

Review effort: Balanced
Findings: 4 Medium severity · 1 Low severity

Open (5)
Resolved since last review (3)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Remove unrelated third-party CLI workflow from this PR

.github/​workflows/​copilot-setup-steps.yml:36

This introduces a permanent third-party CLI installation workflow that is unrelated to the per-call transport change described by the PR. Because the workflow is triggered for every lib/netstd/** push and pull request but only prints the CLI version (it runs neither transport tests nor openspec validate), it adds recurring CI/network cost without validating this feature; move the OpenSpec agent/setup infrastructure to a separate PR or remove it here.

Medium severity Dispose returned transport when wrapper construction fails

lib/​netstd/​Thrift/​Transport/​Layered/​TBufferedTransport.cs:105

If constructing the buffered wrapper throws after the provider has returned its call-owned transport (for example, while reading a custom transport's Configuration), that transport is leaked. This method owns the returned transport and should dispose it on wrapper-construction failure, as the framed implementation does.

Comment thread lib/netstd/Thrift/Transport/Layered/TFramedTransport.cs Outdated
Copilot AI balanced review requested due to automatic review settings October 6, 2026 21:34

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.

Copilot review overview

🔵 Needs a closer look

Buffered transport construction can leak acquired resources, and packaging and CI scope issues remain unresolved.

Review effort: Balanced
Findings: 4 Medium severity

Open (4)
Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Avoid running Copilot setup on every netstd push and pull request

.github/​workflows/​copilot-setup-steps.yml:17

These triggers run a global npm installation on every future netstd push and pull request, but the job only prints the CLI version and does not validate the checked-in specs or source. That adds an unrelated external dependency and CI job to all netstd changes; keep this Copilot setup workflow dispatch-only, or turn it into actual OpenSpec validation in a separately scoped tooling change.

Client: cpp,netstd

Add opt-in per-call transports for generated asynchronous clients, isolating
HTTP and layered transport state while preserving existing shared-protocol
constructors.

Preserve caller-configured HTTP timeouts and propagate the linked cancellation
token through generated request and response operations. Add focused transport,
lifecycle, concurrency, timeout, generator, and tutorial coverage.

Co-Authored-By: GitHub Copilot
Copilot AI balanced review requested due to automatic review settings October 6, 2026 21:47
@birschick-bq
birschick-bq force-pushed the dev/birschick-bq/v3/thrift-5830 branch from 44823b1 to 01c379e Compare October 6, 2026 21:47

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.

Copilot review overview

🔵 Needs a closer look

The cross-cutting generator, concurrency, timeout, and resource-lifetime changes warrant final human validation across supported targets.

Review effort: Balanced
Findings: None

Resolved since last review (4)

@birschick-bq

Copy link
Copy Markdown
Contributor Author

@Jens-G
Note: AI/OpenSpec artifacts are completely optional. Let me know if you want to keep these or have me remove them.

This branch has not been deployed

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

Labels

build and general CI cmake, automake and build system changes c# Pull requests that update C# code Pull requests that update .NET code compiler github_actions Pull requests that update GitHub Actions code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants