Copilot commented on code in PR #3751:
URL: https://github.com/apache/thrift/pull/3751#discussion_r3871483998
##########
lib/cpp/src/thrift/transport/THttpClient.cpp:
##########
@@ -72,10 +82,20 @@ void THttpClient::parseHeader(char* header) {
} else if (boost::istarts_with(header, "Content-Length")) {
chunked_ = false;
contentLength_ = atoi(value);
+ } else if (boost::istarts_with(header, "Connection")) {
+ std::vector<string> options;
+ boost::split(options, value, boost::is_any_of(","));
+ for (const string& option : options) {
+ if (boost::iequals(boost::trim_copy(option), "close")) {
+ closeAfterResponse_ = true;
+ break;
+ }
+ }
}
Review Comment:
`boost::istarts_with(header, "Connection")` can match non-standard header
names that merely begin with "Connection" (e.g., "Connection-Timeout") and
incorrectly treat them as hop-by-hop connection directives. Consider parsing
the field-name up to the ':' and comparing it case-insensitively to exactly
"Connection" (same for other header checks if you want consistent correctness),
then parse the field-value. This avoids false positives and keeps header
handling RFC-aligned.
##########
lib/cpp/src/thrift/transport/THttpClient.cpp:
##########
@@ -38,23 +39,32 @@ THttpClient::THttpClient(std::shared_ptr<TTransport>
transport,
std::string path,
std::shared_ptr<TConfiguration> config)
: THttpTransport(transport, config),
- host_(host),
+ host_(host),
path_(path),
- onewayResponsePending_(false) {
-}
+ onewayResponsePending_(false),
+ closeAfterResponse_(false) {}
-THttpClient::THttpClient(string host, int port, string path,
- std::shared_ptr<TConfiguration> config)
+THttpClient::THttpClient(string host, int port, string path,
std::shared_ptr<TConfiguration> config)
: THttpTransport(std::shared_ptr<TTransport>(new TSocket(host, port)),
config),
host_(host),
path_(path),
- onewayResponsePending_(false) {
-}
+ onewayResponsePending_(false),
+ closeAfterResponse_(false) {}
THttpClient::~THttpClient() = default;
void THttpClient::close() {
onewayResponsePending_ = false;
+ closeAfterResponse_ = false;
+ readBuffer_.resetBuffer();
+ readHeaders_ = true;
+ chunked_ = false;
+ chunkedDone_ = false;
+ chunkSize_ = 0;
+ contentLength_ = 0;
+ httpPos_ = 0;
+ httpBufLen_ = 0;
+ httpBuf_[0] = '\0';
THttpTransport::close();
}
Review Comment:
`THttpClient::close()` is directly resetting a broad set of HTTP parsing
internals (many of which appear to be owned by the `THttpTransport`
implementation). This creates tight coupling and makes it easy to miss newly
introduced state in the future (leading to partial resets and hard-to-debug
behavior). A more maintainable approach is to encapsulate these resets in a
single “reset HTTP parser state” helper (ideally in the owning class) and call
it from both `close()` and any other reconnection path. At minimum, adding a
short comment explaining which invariants must hold after `close()` would help
future maintainers.
##########
lib/cpp/test/OneWayHTTPTest.cpp:
##########
@@ -126,13 +127,63 @@ class TServerReadyEventHandler : public
TServerEventHandler, public Monitor {
return nullptr;
}
bool isListening() const { return isListening_; }
- uint64_t acceptedCount() const { return accepted_; }
+ uint64_t acceptedCount() {
+ Synchronized sync(*this);
Review Comment:
This accessor lost its `const` qualifier compared to before, which can make
call sites more restrictive (e.g., if accessed through a `const` handler
pointer/reference). If possible, keep it `const` while still synchronizing
(e.g., by making the underlying lock/mutex mutable in the Monitor base, or
using an atomic counter if locking isn’t otherwise required for correctness).
If `Synchronized` requires non-const, adding a brief comment justifying the
non-const accessor would reduce confusion.
--
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]
