You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
THRIFT-5830: THttpTransport can now be used in interleaved async calls - #3930
This pull request introduces a new per-call transport mechanism for THttpTransport, allowing each logical Thrift call to use a distinct HTTP request/response cycle. This improves resource management, error isolation, and concurrency for HTTP-based Thrift clients. The implementation includes a new interface, a per-call transport class, and comprehensive tests to verify correct behavior and resource disposal.
Per-call transport implementation
Added the ITPerCallTransportProvider interface to define a standard method for creating per-call transports. (lib/netstd/Thrift/Transport/ITPerCallTransportProvider.cs)
Implemented THttpPerCallTransport, a new internal class that manages request/response streams for a single Thrift call, ensuring proper disposal and isolation from the parent transport. (lib/netstd/Thrift/Transport/Client/THttpPerCallTransport.cs)
Updated THttpTransport to implement ITPerCallTransportProvider and provide the CreatePerCallTransportAsync method, which returns a new THttpPerCallTransport for each call. (lib/netstd/Thrift/Transport/Client/THttpTransport.cs) [1][2]
Internal refactoring and supporting methods
Added an internal SendAsync method to THttpTransport to handle sending the request stream and returning the HTTP response, with robust exception handling and resource cleanup. (lib/netstd/Thrift/Transport/Client/THttpTransport.cs)
Testing
Added comprehensive unit tests to THttpTransportTests to verify per-call transport behavior, including resource disposal, error handling, concurrency, and protocol factory integration. (lib/netstd/Tests/Thrift.Tests/Transports/THttpTransportTests.cs)
These changes collectively improve the reliability, testability, and scalability of HTTP-based Thrift transports by supporting per-call isolation and robust resource management.
The last commit switches the existing THttpTransport.FlushAsync to HttpCompletionOption.ResponseHeadersRead as well, so ConnectTimeout no longer limits how long reading the response body can take for any THttpTransport client, not only on the new per-call path. This is Copilot's new comment on line 256, and it holds up in a test. I ran a local server that answers 200 with Content-Length: 64, sends 8 bytes of the body and then nothing more, with ConnectTimeout = 1000, on .NET 10:
on master, FlushAsync fails with TTransportException (Interrupted) after about 1 s;
at 203447d, FlushAsync returns after 27 ms, and the ReadAllAsync after it, with the default token, was still waiting after 8 s. The per-call path behaves the same way.
With ResponseContentRead in both FlushAsync and SendAsync, both paths fail after about 1 s again. Two of the new tests then need another way to make the read fail, because the body is already buffered: PerCallTransport_ReadFailure_IsDisposedByCallOwner and PerCallTransport_ReadFailureAfterFlush_DisposesOwnedResponse. If the body should stay streamed, the reads need a time limit of their own. The _sent and SendAsync changes in the same commit can stay.
Two suggestions, below the bar for an issue but verified:
A small formality for when you next push: CONTRIBUTING.md asks for a single commit per issue, with a Client: netstd line below the subject. None of the three commits has the Client: line, and the last two have no THRIFT-5830: prefix. Squashing them into one commit with that line covers both.
1. All pull requests should contain a single commit per issue, or we will ask you to squash it.
1. The pull request title must begin with the Jira THRIFT ticket identifier if it has an associated ticket, for example:
THRIFT-9999: an example pull request title
1. Commit messages must follow this pattern for code changes (deviations will not be merged):
THRIFT-9999: [summary of fix, one line if possible]
Client: [language(s) affected, comma separated, for example: "cpp,erl,perl"]
The diffhunk:// footnotes in the description look like a generated summary. If an AI tool also helped with the code, CONTRIBUTING.md recommends a Generated-by: or Co-Authored-By: line in the commit and the PR.
The [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) serves as a great summary. It also outlines potential issues that come with generative tools. For the reasons set out in this document, but also to make it absolutely clear to the reviewers, we strongly recommend using either the `Generated-by:` (as recommended by the linked ASF source) or `Co-Authored-By:` clause (widely adopted common practice, e.g. [here](https://issues.apache.org/jira/browse/SPARK-25018) and [here](https://archive.kernel.org/oldwiki/git.wiki.kernel.org/index.php/CommitMessageConventions.html)) in commits and pull requests, if applicable.
And one question: the netstd generator does not use the new interface yet, so a generated client that is shared across calls still sends every call through the request and response buffers of its one THttpTransport, which is the situation THRIFT-5830 describes. adbc-drivers/hiveserver2#99 changes the generated TCLIService.cs by hand, and its description leaves open whether to keep that or to wait for the generator. Is the generator change planned as part of THRIFT-5830, or as a separate ticket?
If ReadAsStreamAsync (including its cancellation overload) fails after SendAsync has returned, _response and _responseReadTimeout remain live and this method exits without disposing them. That leaks the response/connection on a response-body setup failure, and a retry can overwrite the previous response; wrap stream acquisition in cleanup that disposes and clears both fields before rethrowing.
The response-read timeout is implemented by cancelling readToken, but this method lets the resulting OperationCanceledException escape unchanged. THttpTransport.ReadAsync maps the same condition to TTransportException.Interrupted; without the same catch here, callers get different timeout/cancellation behavior depending on whether the transport was created per call.
Nice, but that does not help much. We don't simply take arbitrary code from someone else's PRs filed elsewhere. We accept code legally contributed to the project (i.e. you are legally entitled to contribute it).
Client: netstd
Handles
- [THRIFT-5830: Using THttpTransport using interleaved asynchronous calls may throw exception](https://issues.apache.org/jira/browse/THRIFT-5830)
This pull request introduces a new per-call transport mechanism for `THttpTransport`, allowing each logical Thrift call to use a distinct HTTP request/response cycle. This improves resource management, error isolation, and concurrency for HTTP-based Thrift clients. The implementation includes a new interface, a per-call transport class, and comprehensive tests to verify correct behavior and resource disposal.
* Added the `ITPerCallTransportProvider` interface to define a standard method for creating per-call transports. (`lib/netstd/Thrift/Transport/ITPerCallTransportProvider.cs`)
* Implemented `THttpPerCallTransport`, a new internal class that manages request/response streams for a single Thrift call, ensuring proper disposal and isolation from the parent transport. (`lib/netstd/Thrift/Transport/Client/THttpPerCallTransport.cs`)
* Updated `THttpTransport` to implement `ITPerCallTransportProvider` and provide the `CreatePerCallTransportAsync` method, which returns a new `THttpPerCallTransport` for each call. (`lib/netstd/Thrift/Transport/Client/THttpTransport.cs`)
* Added an internal `SendAsync` method to `THttpTransport` to handle sending the request stream and returning the HTTP response, with robust exception handling and resource cleanup. (`lib/netstd/Thrift/Transport/Client/THttpTransport.cs`)
* Added comprehensive unit tests to `THttpTransportTests` to verify per-call transport behavior, including resource disposal, error handling, concurrency, and protocol factory integration. (`lib/netstd/Tests/Thrift.Tests/Transports/THttpTransportTests.cs`)
These changes collectively improve the reliability, testability, and scalability of HTTP-based Thrift transports by supporting per-call isolation and robust resource management.
Co-authored-by: Claude Sonnet 5
Co-authored-by: GPT-5.6 Luna
Co-authored-by: GPT-6 Luna
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This is a code-level review only. It does not answer the question above about whether the approach is usable and maintainable for the project.
Re-reviewed at 12b9ce621. No blocking issues found. Checked for bugs and CLAUDE.md compliance.
The issue from the last round is fixed. With the same local server (200, Content-Length: 64, 8 body bytes, then silence) and ConnectTimeout = 1000 on .NET 10, the read after FlushAsync now fails with TTransportException (Interrupted) after about 1 s, on the default path and on the per-call path. The single commit with its Client: netstd line and AI trailers, and the generator change, cover the other points from last time.
Three suggestions, below the bar for an issue but verified:
The new body-read deadline takes its budget from two places. THttpTransport.FlushAsync reads the _connectTimeout field, while THttpPerCallTransport.FlushAsync reads the ConnectTimeout property, which returns HttpClient.Timeout whenever there is a client. The two differ when the caller passes in an HttpClient and sets its Timeout afterwards: ConnectTimeout then reports the new value, but the field keeps the default of 30000. I tried new THttpTransport(hc, config, uri) followed by hc.Timeout = TimeSpan.FromSeconds(1), against a server that stalls in the body. On master, FlushAsync fails with Interrupted after about 1 s. On this PR the per-call path also fails after about 1 s, but on the default path the read was still waiting after 8 s. Reading ConnectTimeout in THttpTransport.FlushAsync too gives both paths the same limit.
WriteAsync takes _outputStream under the lock but writes to it outside the lock, with no catch. If Close() runs on another thread in between, it disposes the stream and the caller gets a raw ObjectDisposedException, where ReadAsync, FlushAsync and SendAsync turn that into TTransportException (NotOpen). The window is narrow, since MemoryStream writes complete synchronously.
In THttpPerCallTransport.FlushAsync, the comment says the late hand-over keeps "a subsequent retry" from leaking the previous response. A second FlushAsync on the same instance already throws, because _sent is set before the send ("Require a fresh per-call transport to retry"), so _response and _responseReadTimeout are always null at that point. The retry clause and the two ?.Dispose() calls could go.
Follow-up to the review above: I have now also run the stalled-body check on the .NET Framework, and there the new read timeout does not work. ConnectTimeout no longer bounds a stalled response body on that runtime, on the default path and on the per-call path.
The probe (200, Content-Length: 64, 8 body bytes, then silence; ConnectTimeout = 1000) ran as a net48 program against the netstandard2.0 build, on .NET Framework 4.8 (4.8.3761) under Wine 10:
on master, FlushAsync fails with TTransportException (Interrupted) after about 1 s;
at 12b9ce6, FlushAsync returns, and the read then waits until the server closes the connection, 60 s in the probe, before it fails with Unknown. Both paths behave the same.
On that runtime the pending read of the response stream does not react to the token. In the same probe, both paths fail after about 1.5 s again with this change: register the read token to dispose the stream around the read, in the non-NET5_0_OR_GREATER branch of THttpTransport.ReadAsync and in THttpPerCallTransport.ReadAsync, and report the resulting exception as Interrupted when the read token has fired. The normal and slow-header cases stay as they are.
I'd like to have some documentation or an example of how this is meant to be used. Reading the code, I don't find it intuitive enough.
What would help most:
A section in lib/netstd/README.md. That file is also the README of the NuGet package, so users will look there. From what I can tell from the code, it should answer these questions:
Which constructor turns the per-call mode on? new Calculator.Client(httpTransport, new TBinaryProtocol.Factory()) does. new Calculator.Client(new TBinaryProtocol(httpTransport)) keeps the shared buffers, without any warning.
Which methods use it? The generated methods such as add() do. If you call send_add() and recv_add() yourself, they use the shared protocols.
How do you combine it with TMultiplexedProtocol, given that netstd has no protocol factory for it?
What is shared across calls (the HttpClient, its default headers, ConnectTimeout), and what does each call get for itself (the request and response buffers)?
Are socket transports affected? As far as I can see, they aren't.
A short code snippet would go a long way.
/// doc comments on the three new generated constructors. Choosing the constructor is the decision that matters, and people make it in IntelliSense. A sentence or two on each, saying when the per-call path is used, would do.
Optionally, a small example in the netstd tutorial: one client over HTTP, a few calls in flight at once (Task.WhenAll), and one close at the end. Two things to know first: the tutorial's HTTP server does not start on master at the moment (THRIFT-6374, which has a one-line fix), and no CI job builds tutorial/netstd, so it needs a local build. Please don't build on the client's -mc option; it has a problem of its own (THRIFT-6375).
And two questions, because I'd rather not guess the intent:
If the THttpTransport is wrapped in a TBufferedTransport or TFramedTransport, the per-call mode is silently off, because the wrapper is not an ITPerCallTransportProvider. Is that intended, or should it throw?
What is the five-argument constructor (inputProtocol, outputProtocol, transport, inputProtocolFactory, outputProtocolFactory) for?
Update on item 3: THRIFT-6374 and THRIFT-6375 are merged, so on master the tutorial's HTTP server now starts, and -mc gives each client its own transport.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c#Pull requests that update C# code Pull requests that update .NET codecompiler
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mergeable
Bot
added
the
c#
Handles
This pull request introduces a new per-call transport mechanism for
THttpTransport, allowing each logical Thrift call to use a distinct HTTP request/response cycle. This improves resource management, error isolation, and concurrency for HTTP-based Thrift clients. The implementation includes a new interface, a per-call transport class, and comprehensive tests to verify correct behavior and resource disposal.Per-call transport implementation
ITPerCallTransportProviderinterface to define a standard method for creating per-call transports. (lib/netstd/Thrift/Transport/ITPerCallTransportProvider.cs)THttpPerCallTransport, a new internal class that manages request/response streams for a single Thrift call, ensuring proper disposal and isolation from the parent transport. (lib/netstd/Thrift/Transport/Client/THttpPerCallTransport.cs)THttpTransportto implementITPerCallTransportProviderand provide theCreatePerCallTransportAsyncmethod, which returns a newTHttpPerCallTransportfor each call. (lib/netstd/Thrift/Transport/Client/THttpTransport.cs) [1] [2]Internal refactoring and supporting methods
SendAsyncmethod toTHttpTransportto handle sending the request stream and returning the HTTP response, with robust exception handling and resource cleanup. (lib/netstd/Thrift/Transport/Client/THttpTransport.cs)Testing
THttpTransportTeststo verify per-call transport behavior, including resource disposal, error handling, concurrency, and protocol factory integration. (lib/netstd/Tests/Thrift.Tests/Transports/THttpTransportTests.cs)These changes collectively improve the reliability, testability, and scalability of HTTP-based Thrift transports by supporting per-call isolation and robust resource management.
Co-authored-by: Claude Sonnet 5
Co-authored-by: GPT 5.6 Luna
Co-authored-by: Copilot Autofix powered by AI 175728472+Copilot@users.noreply.github.com
[skip ci]anywhere in the commit message to free up build resources.