Jens-G commented on PR #3930:
URL: https://github.com/apache/thrift/pull/3930#issuecomment-5785495277
### Code review
Found 1 issue:
1. 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 203447d3f, `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.
https://github.com/apache/thrift/blob/203447d3fd4bbcf82d4399c8b7528d784ec8a601/lib/netstd/Thrift/Transport/Client/THttpTransport.cs#L252-L257
https://github.com/apache/thrift/blob/203447d3fd4bbcf82d4399c8b7528d784ec8a601/lib/netstd/Thrift/Transport/Client/THttpTransport.cs#L341-L346
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.
https://github.com/apache/thrift/blob/203447d3fd4bbcf82d4399c8b7528d784ec8a601/CONTRIBUTING.md#L17-L27
- 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.
https://github.com/apache/thrift/blob/203447d3fd4bbcf82d4399c8b7528d784ec8a601/CONTRIBUTING.md#L116-L119
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?
https://github.com/apache/thrift/blob/203447d3fd4bbcf82d4399c8b7528d784ec8a601/compiler/cpp/src/thrift/generate/t_netstd_generator.cc#L2503-L2505
🤖 Generated with [Claude Code](https://claude.ai/code)
--
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]
