Jens-G commented on PR #3930: URL: https://github.com/apache/thrift/pull/3930#issuecomment-5846691071
### Code review 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. https://github.com/apache/thrift/blob/12b9ce6218c78870964089e0ca594d74ae0b136c/lib/netstd/Thrift/Transport/Client/THttpTransport.cs#L337-L341 https://github.com/apache/thrift/blob/12b9ce6218c78870964089e0ca594d74ae0b136c/lib/netstd/Thrift/Transport/Client/THttpPerCallTransport.cs#L102-L104 - `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. https://github.com/apache/thrift/blob/12b9ce6218c78870964089e0ca594d74ae0b136c/lib/netstd/Thrift/Transport/Client/THttpTransport.cs#L261-L269 - 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. https://github.com/apache/thrift/blob/12b9ce6218c78870964089e0ca594d74ae0b136c/lib/netstd/Thrift/Transport/Client/THttpPerCallTransport.cs#L141-L156 🤖 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]
