Skip to content

THRIFT-5830: THttpTransport can now be used in interleaved async calls - #3978

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

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

Conversation

@birschick-bq

@birschick-bq birschick-bq commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

THRIFT-5830: Using THttpTransport using interleaved asynchronous calls may throw exception
Client: netstd

feat: Implement per-call transport support in THttpTransport

  • Added ITPerCallTransportProvider interface to define per-call transport creation.
  • Enhanced TBaseClient to support per-call transport with separate protocols.
  • Introduced THttpPerCallTransport for handling HTTP requests on a per-call basis.
  • Updated THttpTransport to manage lifecycle and creation of per-call transports.
  • Improved error handling and resource management in transport classes.
  • Updated tutorial with per-call semantics. Note: requires new version of Thrift.exe

Co-authored-by: Claude Sonnet 5
Co-authored-by: GPT 5.6 Luna
Co-authored-by: GitHub Copilot

  • 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.

@mergeable mergeable Bot added c# Pull requests that update C# code Pull requests that update .NET code compiler labels Sep 29, 2026
@birschick-bq
birschick-bq marked this pull request as ready for review September 29, 2026 03:19
Copilot AI balanced review requested due to automatic review settings September 29, 2026 03:19

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

Legacy .NET response reads can still block beyond configured deadlines, and the shared path may use a stale timeout.

Review effort: Balanced
Findings: 2 High severity · 1 Medium severity

Open (3)
What changed in this PR

Adds isolated per-call HTTP transports so generated .NET clients can safely perform interleaved asynchronous RPCs.

Changes:

  • Adds per-call transport creation and lifecycle management.
  • Updates client generation and protocol scoping for concurrent calls.
  • Adds concurrency, timeout, cancellation, and disposal tests.
File Description
lib/​netstd/​Thrift/​Transport/​ITPerCallTransportProvider.cs Defines per-call transport creation.
lib/​netstd/​Thrift/​Transport/​Client/​THttpTransport.cs Manages shared HTTP resources and per-call transports.
lib/​netstd/​Thrift/​Transport/​Client/​THttpPerCallTransport.cs Implements isolated request/response state.
lib/​netstd/​Thrift/​TBaseClient.cs Scopes protocols to each asynchronous call.
lib/​netstd/​Tests/​Thrift.Tests/​Transports/​THttpTransportTests.cs Tests concurrency and lifecycle behavior.
compiler/​cpp/​src/​thrift/​generate/​t_netstd_generator.cc Generates constructors and per-call execution wrappers.

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

Comment thread lib/netstd/Thrift/Transport/Client/THttpPerCallTransport.cs Outdated
Comment thread lib/netstd/Thrift/Transport/Client/THttpTransport.cs Outdated
Comment thread lib/netstd/Thrift/Transport/Client/THttpTransport.cs Outdated
Copilot AI review requested due to automatic review settings September 29, 2026 03:59

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

Response-body timeouts remain ineffective on .NET Framework for both HTTP read paths.

Review effort: Balanced
Findings: 2 High severity

Open (2)
Resolved since last review (1)

Copilot AI review requested due to automatic review settings September 29, 2026 04:24
@birschick-bq
birschick-bq force-pushed the dev/birschick-bq/v2/thrift-5830 branch from 9f68c22 to e5563ba Compare September 29, 2026 04:24

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

The commit message must use the repository-required THRIFT-5830: subject and Client: netstd line.

Review effort: Balanced
Findings: 1 High severity · 1 Low severity

Open (2)
Resolved since last review (1)

Comment thread lib/netstd/Thrift/Transport/Client/THttpTransport.cs

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

The new legacy .NET cancellation branch lacks runtime test coverage.

Review effort: Balanced
Findings: 2 Low severity

Open (2)
Resolved since last review (1)

Comment thread lib/netstd/Thrift/Transport/Client/THttpTransport.cs
@birschick-bq
birschick-bq force-pushed the dev/birschick-bq/v2/thrift-5830 branch from 7216eb3 to 690ba28 Compare September 29, 2026 16:44
Copilot AI review requested due to automatic review settings September 29, 2026 16:44

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

The public provider path does not validate null transports and can mask failures with a secondary exception.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)

Comment thread lib/netstd/Thrift/TBaseClient.cs Outdated
Copilot AI review requested due to automatic review settings September 29, 2026 16:52

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

The close/write race and commit-format issue should be resolved before approval.

Review effort: Balanced
Findings: 1 Low severity

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

In code that hasn't changed since last review

Medium severity Normalize stream-close race to the transport NotOpen error

lib/​netstd/​Thrift/​Transport/​Client/​THttpTransport.cs:293

Close() can dispose this captured stream after the lifecycle lock is released but before WriteAsync starts, causing a raw ObjectDisposedException instead of the transport's NotOpen error. Normalize that close race as the other I/O paths do.

Comment thread lib/netstd/Thrift/TBaseClient.cs

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

Cross-target cancellation and concurrent resource-lifecycle behavior warrant final human validation.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@birschick-bq
birschick-bq force-pushed the dev/birschick-bq/v2/thrift-5830 branch from aed8a5b to c43e723 Compare October 2, 2026 19:57
@birschick-bq
birschick-bq force-pushed the dev/birschick-bq/v2/thrift-5830 branch 3 times, most recently from 4129483 to 6cf177d Compare October 2, 2026 21:50
…s may throw exception

Client: netstd

feat: Implement per-call transport support in THttpTransport

- Added ITPerCallTransportProvider interface to define per-call transport creation.
- Enhanced TBaseClient to support per-call transport with separate protocols.
- Introduced THttpPerCallTransport for handling HTTP requests on a per-call basis.
- Updated THttpTransport to manage lifecycle and creation of per-call transports.
- Improved error handling and resource management in transport classes.
- Updated tutorial with per-call semantics. Note: requires new version of Thrift.exe

Co-authored-by: Claude Sonnet 5
Co-authored-by: GPT 5.6 Luna
Co-authored-by: GitHub Copilot
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@birschick-bq
birschick-bq force-pushed the dev/birschick-bq/v2/thrift-5830 branch from 6cf177d to dc3f8f7 Compare October 2, 2026 22:03
@birschick-bq

Copy link
Copy Markdown
Contributor Author

@Jens-G
I've addressed the comments in the comment on the abandoned PR
#3930 (comment)

@birschick-bq

Copy link
Copy Markdown
Contributor Author

abandoning this change.

@Jens-G

Jens-G commented Oct 11, 2026 •

Copy link
Copy Markdown
Member

I didnt have the time to look into it yet. You're giving up on this or you simply don't want to wait? @birschick-bq

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

Labels

c# Pull requests that update C# code Pull requests that update .NET code compiler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants