hjwsm1989 commented on code in PR #3690:
URL: https://github.com/apache/brpc/pull/3690#discussion_r4422328960
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -155,19 +144,13 @@ static int WriteAck(Socket* s, const void* data, size_t
n) {
butil::IOBuf sendbuf;
sendbuf.append(data, n);
Socket::WriteOptions wopt;
- // These writes historically ignored EOVERCROWDED so control replies
- // could always be queued, but that defeats the explicit memory bound of
- // -socket_max_unwritten_bytes: a peer flooding PING/SETTINGS frames
- // while withholding TCP reads makes this process buffer ack frames
- // without limit. Respect the bound by default; callers treat a failed
- // ack write as a connection error and close the overcrowded connection.
- wopt.ignore_eovercrowded = FLAGS_h2_ack_ignore_eovercrowded;
+ wopt.ignore_eovercrowded = true;
Review Comment:
Fixed in commit `62743640`: the previous force-push accidentally checked out
`http2_rpc_protocol.{h,cpp}` from a `1.18`-based branch and reverted recent
`master` changes. Restored all upstream `master` logic.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -590,11 +594,7 @@ ParseResult H2Context::Consume(
LOG(WARNING) << "Fail to send GOAWAY to " << *_socket;
return MakeParseError(PARSE_ERROR_ABSOLUTELY_WRONG);
}
- // https://tools.ietf.org/html/rfc7540#section-5.4.1
- // A connection error is unrecoverable: close the connection
- // after sending GOAWAY instead of continuing to parse (and
- // buffer) whatever the misbehaving peer keeps sending.
- return MakeParseError(PARSE_ERROR_ABSOLUTELY_WRONG);
+ return MakeMessage(NULL);
Review Comment:
Fixed in commit `62743640`: the previous force-push accidentally checked out
`http2_rpc_protocol.{h,cpp}` from a `1.18`-based branch and reverted recent
`master` changes. Restored all upstream `master` logic.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -797,26 +760,18 @@ H2ParseResult H2StreamContext::OnContinuation(
<< ", stream_id=" << frame_head.stream_id;
return MakeH2Error(H2_PROTOCOL_ERROR);
}
- // See the same check in H2StreamContext::OnHeaders().
- if (_rejected_error != H2_NO_ERROR) {
- return MakeH2Error(_rejected_error, stream_id());
- }
if (_stream_ended) {
return OnEndStream();
}
}
- return MakeH2Message(nullptr);
+ return MakeH2Message(NULL);
}
H2ParseResult H2Context::OnData(
butil::IOBufBytesIterator& it, const H2FrameHead& frame_head) {
uint32_t frag_size = frame_head.payload_size;
uint8_t pad_length = 0;
if (frame_head.flags & H2_FLAGS_PADDED) {
Review Comment:
Fixed in commit `62743640`: the previous force-push accidentally checked out
`http2_rpc_protocol.{h,cpp}` from a `1.18`-based branch and reverted recent
`master` changes. Restored all upstream `master` logic.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1076,12 +1045,10 @@ H2ParseResult H2Context::OnGoAway(
LOG(ERROR) << "Invalid flags=" << h.flags;
return MakeH2Error(H2_PROTOCOL_ERROR);
}
- // Last-Stream-ID and Error Code precede the Additional Debug Data.
- // The reserved bit of Last-Stream-ID is ignored on receipt.
- const int last_stream_id = static_cast<int>(LoadUint32(it) & 0x7FFFFFFF);
- const H2Error ALLOW_UNUSED h2_error = static_cast<H2Error>(LoadUint32(it));
// Skip Additional Debug Data
it.forward(h.payload_size - 8);
+ const int last_stream_id = static_cast<int>(LoadUint32(it));
+ const H2Error ALLOW_UNUSED h2_error = static_cast<H2Error>(LoadUint32(it));
Review Comment:
Fixed in commit `62743640`: the previous force-push accidentally checked out
`http2_rpc_protocol.{h,cpp}` from a `1.18`-based branch and reverted recent
`master` changes. Restored all upstream `master` logic.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1222,10 +1189,6 @@ void H2Context::DeferWindowUpdate(int64_t size) {
SaveUint32(winbuf + FRAME_HEAD_SIZE, conn_wu);
if (WriteAck(_socket, winbuf, sizeof(winbuf)) != 0) {
LOG(WARNING) << "Fail to send WINDOW_UPDATE";
- // Retry on a later DATA frame instead of silently losing
- // the window bytes (the peer would stall otherwise).
- _deferred_window_update.fetch_add(
- conn_wu, butil::memory_order_relaxed);
}
Review Comment:
Fixed in commit `62743640`: the previous force-push accidentally checked out
`http2_rpc_protocol.{h,cpp}` from a `1.18`-based branch and reverted recent
`master` changes. Restored all upstream `master` logic.
##########
src/brpc/policy/http2_rpc_protocol.cpp:
##########
@@ -1338,13 +1300,6 @@ void H2StreamContext::SetState(H2StreamState state) {
int H2StreamContext::ConsumeHeaders(butil::IOBufBytesIterator& it) {
HPacker& hpacker = _conn_ctx->hpacker();
HttpHeader& h = header();
- // https://tools.ietf.org/html/rfc7540#section-10.5.1
- // Bound the cumulative decoded size of the header list. Without this
- // check nothing limits header bytes (-max_body_size covers DATA only)
- // and 1-byte HPACK indexed references to a large dynamic-table entry
- // amplify a small HEADERS/CONTINUATION frame ~4000x ("HPACK bomb").
- const uint32_t max_header_list_size =
- _conn_ctx->_unack_local_settings.max_header_list_size;
while (it) {
Review Comment:
Fixed in commit `62743640`: the previous force-push accidentally checked out
`http2_rpc_protocol.{h,cpp}` from a `1.18`-based branch and reverted recent
`master` changes. Restored all upstream `master` logic.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
