Copilot commented on code in PR #4000:
URL: https://github.com/apache/thrift/pull/4000#discussion_r4200146679
##########
lib/netstd/.github/agents/openspec.agent.md:
##########
@@ -0,0 +1,74 @@
+---
+name: OpenSpec
+description: "Manages OpenSpec changes, specs, and workflows using the
OpenSpec CLI. Use this agent for proposing changes, exploring ideas, validating
artifacts, checking status, and archiving completed work."
Review Comment:
Repository custom agents are discovered from the repository-root
`.github/agents` directory. This nested `lib/netstd/.github/agents` file will
not be loaded, so the OpenSpec agent is unavailable; relocate it to the root
agent directory.
##########
lib/netstd/.github/workflows/copilot-setup-steps.yml:
##########
@@ -0,0 +1,32 @@
+# Generated by OpenSpec for GitHub Copilot coding agent support.
+
+name: "Copilot Setup Steps"
Review Comment:
GitHub only discovers Actions workflows in the repository-root
`.github/workflows` directory. At `lib/netstd/.github/workflows`, this job is
never registered, so the claimed validation and Copilot setup do not run; move
it to the root workflow directory and update its path filters.
##########
lib/netstd/Thrift/Transport/Layered/TBufferedTransport.cs:
##########
@@ -24,7 +24,7 @@
namespace Thrift.Transport
{
// ReSharper disable once InconsistentNaming
- public class TBufferedTransport : TLayeredTransport
+ public class TBufferedTransport : TLayeredTransport,
ITPerCallTransportProvider
Review Comment:
`TBufferedTransport` now advertises `ITPerCallTransportProvider` even when
its inner transport does not support it. `TBaseClient` therefore selects the
per-call path for, e.g., buffered TCP, and `PerCallTimeout` throws instead of
falling back to the documented shared behavior. The capability contract needs a
way to report/delegate support conditionally, and `TBaseClient` must only opt
in when that condition is true.
##########
lib/netstd/Thrift/Transport/Layered/TFramedTransport.cs:
##########
@@ -28,7 +28,7 @@
namespace Thrift.Transport
{
// ReSharper disable once InconsistentNaming
- public class TFramedTransport : TLayeredTransport
+ public class TFramedTransport : TLayeredTransport,
ITPerCallTransportProvider
Review Comment:
`TFramedTransport` advertises per-call support for every inner transport.
When the inner transport is unsupported, `TBaseClient` still enters the
per-call path and this wrapper throws, contradicting the shared-mode fallback
documented for transports without the capability. Make support conditional
through the provider contract and client capability check.
##########
lib/netstd/Thrift/Transport/Client/THttpTransport.cs:
##########
@@ -115,31 +130,43 @@ public int ConnectTimeout
public MediaTypeHeaderValue ContentType { get; set; }
- public override Task OpenAsync(CancellationToken cancellationToken)
+ /// <summary>
+ /// Gets the configured timeout for a complete client call using this
transport.
+ /// </summary>
+ public TimeSpan PerCallTimeout => _httpClient?.Timeout ??
TimeSpan.FromMilliseconds(_connectTimeout);
+
+ /// <summary>
+ /// Creates a transport with independent request and response buffers
for one client call.
+ /// </summary>
+ /// <param name="cancellationToken">Token used to cancel transport
creation.</param>
+ /// <returns>A new transport owned by the caller.</returns>
+ public Task<TTransport> CreatePerCallTransportAsync(CancellationToken
cancellationToken)
{
cancellationToken.ThrowIfCancellationRequested();
- return Task.CompletedTask;
- }
+ if (_isDisposed || _httpClient == null)
+ throw new ObjectDisposedException(nameof(THttpTransport));
- public override void Close()
- {
- if (_inputStream != null)
+ _httpClientLease.AddReference();
+ try
{
- _inputStream.Dispose();
- _inputStream = null;
+ return Task.FromResult<TTransport>(new
THttpTransport(_httpClientLease, Configuration, _uri));
Review Comment:
The call-scoped transport does not copy the root transport's public
`ContentType`, so callers that configured a custom Thrift media type silently
send `application/x-thrift` in per-call mode. Preserve this per-transport
setting when cloning the transport.
##########
lib/netstd/per-call-transport.md:
##########
@@ -0,0 +1,30 @@
+# Per-Call Transport
Review Comment:
`lib/netstd/Makefile.am:75-107` explicitly lists files included in source
distributions, and this new top-level document is absent. Add
`per-call-transport.md` to `EXTRA_DIST` so it is shipped in release archives.
##########
lib/netstd/.github/workflows/copilot-setup-steps.yml:
##########
@@ -0,0 +1,32 @@
+# Generated by OpenSpec for GitHub Copilot coding agent support.
+
+name: "Copilot Setup Steps"
+
+# Runs automatically when changed (for validation) and can be triggered
manually.
+on:
+ workflow_dispatch:
+ push:
+ paths:
+ - .github/workflows/copilot-setup-steps.yml
+ pull_request:
+ paths:
+ - .github/workflows/copilot-setup-steps.yml
+
+jobs:
+ # The job MUST be called `copilot-setup-steps` for Copilot coding agent to
pick it up.
+ copilot-setup-steps:
+ runs-on: ubuntu-latest
+ timeout-minutes: 10
+
+ permissions:
+ contents: read
+
+ steps:
+ - name: Checkout code
+ uses: actions/checkout@v4
Review Comment:
This workflow uses a moving action tag, while the repository consistently
pins `actions/checkout` to a full SHA (for example
`.github/workflows/build.yml:51` and `.github/workflows/cmake.yml:23`). Use the
repository's pinned revision to keep workflow execution reproducible.
##########
tutorial/netstd/README.md:
##########
@@ -77,6 +77,9 @@ Usage:
Client -tr:<transport> -pr:<protocol> -mc:<numClients>
will run client with specified arguments (tcp transport and binary
protocol by default)
+ Client -tr:http -per-call [-mc:<numClients>]
+ will run concurrent HTTP calls through one per-call-enabled generated
client
Review Comment:
With `-mc` greater than one, the implementation creates one generated client
per task, and each client makes concurrent calls; it does not route all calls
through one client. Adjust this description so the documented behavior matches
the tutorial.
##########
lib/netstd/.github/workflows/copilot-setup-steps.yml:
##########
@@ -0,0 +1,32 @@
+# Generated by OpenSpec for GitHub Copilot coding agent support.
+
+name: "Copilot Setup Steps"
+
+# Runs automatically when changed (for validation) and can be triggered
manually.
+on:
+ workflow_dispatch:
+ push:
+ paths:
+ - .github/workflows/copilot-setup-steps.yml
+ pull_request:
+ paths:
+ - .github/workflows/copilot-setup-steps.yml
+
+jobs:
+ # The job MUST be called `copilot-setup-steps` for Copilot coding agent to
pick it up.
+ copilot-setup-steps:
+ runs-on: ubuntu-latest
+ timeout-minutes: 10
+
+ permissions:
+ contents: read
+
+ steps:
+ - name: Checkout code
+ uses: actions/checkout@v4
+
+ - name: Install OpenSpec CLI
+ run: npm install -g @fission-ai/openspec
Review Comment:
Installing the unversioned latest OpenSpec package makes this setup change
behavior without any repository change and can break when a new release is
published. Pin an audited exact package version here.
##########
lib/netstd/per-call-transport.md:
##########
@@ -0,0 +1,30 @@
+# Per-Call Transport
+
+Create an extension to existing transports so that per-call semantics are
enabled such that
+asynchronous and interleaved calls are safe and can implement a read-ahead
buffering protocol.
+
+## TDD Discipline (non-negotiable)
+- Follow Red–Green–Refactor.
+- For every behavior in the specs, write failing tests FIRST, then implement.
+- Task lists must explicitly sequence: tests → implementation → refactor.
+
+## Features
+
+### per-call-transport
+
+- add a new mechanism for per-call transport to support the existing
THttpTransport and any wrapping transports that can support or delegate the
per-call semantics.
+- reference the observed bug from
https://issues.apache.org/jira/browse/THRIFT-5830 where interleaved and
asynchronous call can cause exceptions due to the shared buffers being created
and disposed before the previous call can retrieve its contents.
+- When using the THttpTransport, interleaved asynchronous calls can cause the
following exception: System.AggregateException : One or more errors occurred.
Couldn't connect to server: System.Net.Http.HttpRequestException: Error while
copying content to a stream. ---> System.ObjectDisposedException: Cannot
access a closed Stream.
+- Essentially, the THttpTransport._outputStream can get disposed while another
asynchronous call is running. This is due to allocating the `_outputStream` at
the class level. The stream should be allocated on a call-by-call basis.
+- backward compatibility must be maintained for the shared buffer semantics.
+- ensure race conditions are handled safely.
+- ensure disposing buffers and clients are done safefly in both the happy path
and any situation where an exception may interupt the execution path.
Review Comment:
Correct the two spelling errors in this requirement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
