deps: update grpc to 1.74.0#350
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughBatches of changes add channelz v2 protobufs and property-list APIs, expand credentials (JWT token-file, call-creds registry), introduce endpoint/fd-based channel creation, refactor channelz and transports (HTTP/2 and chaotic_good) for telemetry and scheduling, update build/versioning, and add many generated upb artifacts. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant grpcpp (experimental)
participant grpc_core
participant EventEngine
participant Http2Transport
participant Channelz
participant JwtFileCreds
App->>grpcpp (experimental): CreateChannelFromEndpoint(endpoint/fd, creds, args)
grpcpp (experimental)->>grpc_core: CreateChannelFromEndpoint (takes ownership)
grpc_core->>EventEngine: Query GetTelemetryInfo() / hand off Endpoint
grpc_core->>Http2Transport: initialize transport (legacy or promise-based)
Http2Transport->>EventEngine: attach endpoint, create WriteEventSink
Http2Transport->>Channelz: AddData(PropertyList)
App->>grpc_core: Make RPC (uses channel creds)
grpc_core->>JwtFileCreds: Fetch token (async FileReader)
JwtFileCreds-->>grpc_core: token / expiration (callback)
grpc_core->>Channelz: Update node trace/properties (AddData / ChannelTrace)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes This diff is large, heterogeneous, and touches core subsystems (transports, credentials, channelz, event engine, build), plus many generated files — thorough review across multiple domains is required. Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 31
🔭 Outside diff range comments (2)
deps/grpc/src/core/ext/transport/chttp2/transport/http2_status.h (1)
315-316: Explicitly handlekHTTP11Requiredin the string mappingThe dedicated
kHTTP11Requiredvalue (0xd) is never matched in theErrorCodeToStringswitch, so it falls through to theDCHECK(false)(and isn’t returned as"HTTP_1_1_REQUIRED"). Instead onlykDoNotUse(0xff) returns that literal. We should add an explicit case forkHTTP11Requiredbefore thekDoNotUseclause and adjust the latter accordingly.• File: deps/grpc/src/core/ext/transport/chttp2/transport/http2_status.h
– Around line 315, in the switch inside theErrorCodeToString(or similarly named) inline function.Suggested diff:
@@ -312,7 +312,11 @@ inline const char* ErrorCodeToString(Http2ErrorCode error) { case Http2ErrorCode::kInadequateSecurity: return "INADEQUATE_SECURITY"; + case Http2ErrorCode::kHTTP11Required: + return "HTTP_1_1_REQUIRED"; case Http2ErrorCode::kDoNotUse: - return "HTTP_1_1_REQUIRED"; + // Should never be hit except via default clauses; leave for future-proofing + break; default: DCHECK(false); return "UNKNOWN_HTTP2_ERROR";deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h (1)
308-323: Implement stream ID exhaustion handling.The TODO indicates that stream ID exhaustion is not handled. This could cause undefined behavior when stream IDs exceed the maximum value.
The function should return an error status when stream IDs are exhausted:
uint32_t NextStreamId( InterActivityMutex<uint32_t>::Lock& next_stream_id_lock) { const uint32_t stream_id = *next_stream_id_lock; if (stream_id > RFC9113::kMaxStreamId31Bit) { - // TODO(tjagtap) : [PH2][P3] : Handle case if transport runs out of stream - // ids + return 0; // Or throw/return error } (*next_stream_id_lock) += 2; return stream_id; }
🧹 Nitpick comments (27)
deps/grpc/src/core/channelz/zviz/strings.cc (1)
21-27: LGTM! Clean string mapping implementation.The
DisplayKindfunction correctly maps internal entity kinds to user-friendly display strings with appropriate fallbacks for empty and unknown inputs.For future scalability, consider using a static map if more entity kinds are added:
std::string DisplayKind(absl::string_view kind) { if (kind.empty()) return "Entity"; static const auto* kind_map = new absl::flat_hash_map<std::string, std::string>{ {"channel", "Channel"}, {"subchannel", "Subchannel"}, {"socket", "Socket"}, }; auto it = kind_map->find(kind); return it != kind_map->end() ? it->second : absl::StrCat("Entity kind '", kind, "'"); }deps/grpc/src/core/credentials/call/jwt_util.cc (1)
51-68: Consider adding validation for the exp field valueThe function correctly extracts the exp field but doesn't validate its value. Consider adding checks for:
- Whether exp is 0 (which could indicate a missing or invalid expiration)
- Whether the timestamp represents a reasonable future time
- Whether the token has already expired
deps/grpc/src/core/channelz/zviz/layout_html.h (1)
47-50: Consider adding bounds checking for table operations.The
NewRow()method increments row index without apparent bounds checking. Consider adding validation or documentation about expected table size limits.deps/grpc/Makefile (1)
1864-1866: Hard-coded SONAME will need manual edits next timeThe link step bakes in
-Wl,-soname,libgrpc.so.49.
WhenCORE_VERSIONis bumped again, forgetting to adjust this constant will leave an incorrect SONAME while the file name changes.Consider deriving the major part from
$(CORE_VERSION)instead, e.g.:-Wl,-soname,libgrpc.so.49 +Wl,-soname,libgrpc.so.$(firstword $(subst ., ,$(CORE_VERSION)))This keeps the rule self-maintaining.
deps/grpc/src/core/ext/transport/chttp2/transport/message_assembler.h (1)
79-80: Consider logging level for production systemsWhile the error logging is helpful for debugging, consider whether
LOG(ERROR)is the appropriate level for incomplete gRPC messages, as this might be noisy in production systems where such conditions could occur during normal operation (e.g., network issues).deps/grpc/src/core/channelz/zviz/data.cc (1)
31-66: LGTM: Comprehensive PropertyValue formattingThe switch statement properly handles all PropertyValue types with appropriate layout intents. The recursive call for
any_valueis correct, though consider adding recursion depth protection if deeply nested structures are possible.Consider adding recursion depth protection for the
any_valuecase to prevent stack overflow with deeply nested structures:void Format(Environment& env, const grpc::channelz::v2::PropertyValue& value, layout::Element& element, int depth = 0) { if (depth > MAX_RECURSION_DEPTH) { element.AppendText(layout::Intent::kNote, "Max recursion depth exceeded"); return; } // ... existing switch logic ... case grpc::channelz::v2::PropertyValue::kAnyValue: Format(env, value.any_value(), element, depth + 1); break; }deps/grpc/src/core/ext/transport/chaotic_good/scheduler.h (1)
15-68: Well-designed scheduler interface with minor improvement opportunities.The two-phase scheduling design is clean and well-documented. A few suggestions for improvement:
- Consider marking
Config()asconstsince it shouldn't modify the object state- Consider using string_view or const reference for the
nameparameter inSetConfigApply these improvements:
- virtual void SetConfig(absl::string_view name, absl::string_view value) = 0; + virtual void SetConfig(absl::string_view name, absl::string_view value) = 0; - virtual std::string Config() const = 0; + virtual std::string Config() const = 0;Note: The first suggestion about SetConfig parameters is already optimal - string_view is the right choice here.
deps/grpc/src/core/credentials/transport/google_default/google_default_credentials.cc (1)
342-399: Well-structured credentials wrapper with minor improvements needed.The
GoogleDefaultCallCredentialsWrappercorrectly implements dynamic credential selection based on transport type. Consider these improvements:
- Add validation in the constructor to ensure at least one credential is non-null
- Improve the comparison implementation similar to other credential types
GoogleDefaultCallCredentialsWrapper( grpc_core::RefCountedPtr<grpc_call_credentials> tls_credentials, grpc_core::RefCountedPtr<grpc_call_credentials> alts_credentials) : tls_credentials_(std::move(tls_credentials)), - alts_credentials_(std::move(alts_credentials)) {}; + alts_credentials_(std::move(alts_credentials)) { + CHECK(tls_credentials_ != nullptr || alts_credentials_ != nullptr); + }deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.cc (1)
40-43: Document the cancellation limitation.The inability to cancel file reading operations could be a concern for shutdown scenarios. Consider documenting this limitation in the class or method documentation.
deps/grpc/src/core/channelz/zviz/html.cc (1)
79-79: Add explicit include for std::tuple.The code uses
std::tuplebut doesn't explicitly include<tuple>. While it may work due to transitive includes, it's better to be explicit.Add the following include at the top of the file:
+#include <tuple>deps/grpc/src/core/ext/transport/chttp2/client/chttp2_connector.cc (2)
193-199: Remove duplicate comment about the temporary macro.This comment block is identical to the one at lines 53-59. Consider removing this duplication to improve code readability.
227-227: Use consistent assertion macros.Line 177 uses
CHECK_NEwhile line 227 usesDCHECK_NE. For consistency, consider using the same assertion macro.- DCHECK_NE(result_->transport, nullptr); + CHECK_NE(result_->transport, nullptr);deps/grpc/src/core/credentials/call/call_creds_registry_init.cc (1)
73-77: JSON field name doesn't match the type name.The JSON field is named
"jwt_token_file"while the type returned byType()is"jwt_token_file". This is consistent, but consider if the field should be named"path"for clarity since it represents a file path.deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc (1)
733-742: Consider scheduling removal of the legacy TCP tracing pathThe code maintains both old and new TCP tracing methods. While backwards compatibility is important, having two paths increases maintenance burden. Consider adding a TODO to remove the old path once the new event engine-based approach is fully adopted.
Would you like me to create an issue to track the deprecation timeline for the legacy
CopyContextFnapproach?deps/grpc/src/core/channelz/zviz/layout.h (1)
121-126: Consider adding table metadata methodsThe
Tableinterface is clean and minimal. However, you might want to consider adding methods for table headers or column/row spans in the future if more complex tables are needed.deps/grpc/src/core/channelz/zviz/html.h (1)
48-85: Well-designed fluent HTML builder APIThe
Containerclass provides an excellent fluent API with:
- Template-based item creation for flexibility
- Convenience methods that reduce boilerplate
- Proper memory management using
unique_ptrOne minor observation: the forward declaration on line 67 uses
html::Textwhich seems redundant since we're already in thehtmlnamespace.Container& Text(std::string text) { - NewItem<html::Text>(std::move(text)); + NewItem<::grpc_zviz::html::Text>(std::move(text)); return *this; }deps/grpc/src/core/ext/transport/chaotic_good/scheduler.cc (1)
224-231: Consider adding epsilon to prevent division by very small numbers.The inverse receive time calculations could result in very large weights when the denominator approaches zero. Consider adding a small epsilon value for numerical stability.
return RandomChannel( channels(), bytes, [](const Channel* c, uint64_t bytes) { - return 1.0 / (c->start_time + bytes / c->bytes_per_second); + constexpr double kEpsilon = 1e-9; + return 1.0 / (c->start_time + bytes / c->bytes_per_second + kEpsilon); });deps/grpc/src/core/ext/transport/chaotic_good/client_transport.cc (2)
259-262: Track the TODO for complete channelz data export.The party data export is implemented, but stream dispatch data is missing as noted in the TODO.
Would you like me to open an issue to track adding stream dispatch data to the channelz export?
198-198: Token-based flow control properly integrated.The explicit token counts align with the new scheduling framework. Consider defining a constant for the cancel frame token count instead of using the magic number 1.
- self->outgoing_frames_.UnbufferedImmediateSend( - UntracedOutgoingFrame(CancelFrame{stream_id}), 1); + constexpr size_t kCancelFrameTokens = 1; + self->outgoing_frames_.UnbufferedImmediateSend( + UntracedOutgoingFrame(CancelFrame{stream_id}), kCancelFrameTokens);Also applies to: 274-276, 333-337
deps/grpc/src/core/channelz/channel_trace.cc (1)
77-91: Consider the performance impact of shrink_to_fit.The memory eviction logic is sound, but calling
shrink_to_fit()at line 87 might cause performance issues under memory pressure as it can trigger reallocation. Consider batching or limiting the frequency of shrink operations.- if (GPR_UNLIKELY(current_memory_ > max_memory_)) { - entries_.shrink_to_fit(); - current_memory_ = MemoryUsageOf(entries_); - } + if (GPR_UNLIKELY(current_memory_ > max_memory_)) { + // Only shrink if we've freed significant space + size_t capacity_bytes = entries_.capacity() * sizeof(Entry); + size_t used_bytes = entries_.size() * sizeof(Entry); + if (capacity_bytes > used_bytes * 2) { + entries_.shrink_to_fit(); + current_memory_ = MemoryUsageOf(entries_); + } + }deps/grpc/src/core/channelz/channel_trace.h (1)
29-44: Consider adding missing includes for completeness.The code uses
std::string,std::vector, and other STL containers but doesn't include their headers directly. While they may be included transitively, it's better practice to include them explicitly.#include <stdint.h> #include <sys/types.h> +#include <string> +#include <vector> #include <algorithm> #include <memory> #include <tuple> #include <type_traits>deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h (2)
52-88: Add documentation for DeliveryData fields.The
DeliveryDatastruct fields would benefit from clearer documentation about their units and usage.struct DeliveryData { - // Time in seconds of the time that a byte sent now would be received at the - // peer. + // Unix timestamp (seconds since epoch) when a byte sent now would be received at the peer double start_time; - // The rate of bytes per second that a channel is expected to send. + // Expected send rate in bytes per second (converted from internal bytes/nanosecond) double bytes_per_second; };
330-340: Remove duplicate cancellation logic.The
AwaitPromisedestructor duplicates the exact cancellation logic fromReadTicketdestructor. Consider extracting to a common method.Extract the cancellation logic to avoid duplication:
+ static void CancelCompletion(Completion* completion, InputQueue* queue) { + completion->mu.Lock(); + if (!completion->ready) { + completion->mu.Unlock(); + queue->Cancel(completion); + } else { + completion->mu.Unlock(); + } + } ~ReadTicket() { if (input_queues_ != nullptr) { - completion_->mu.Lock(); - if (!completion_->ready) { - completion_->mu.Unlock(); - input_queues_->Cancel(completion_.get()); - } else { - completion_->mu.Unlock(); - } + CancelCompletion(completion_.get(), input_queues_.get()); } }deps/grpc/src/core/channelz/property_list.h (1)
43-47: Consider using std::unique_ptr instead of std::shared_ptr for OtherPropertyValue.Since
OtherPropertyValueinstances are typically not shared between multiple owners, usingunique_ptrwould provide clearer ownership semantics and better performance.using PropertyValue = std::variant<absl::string_view, std::string, int64_t, uint64_t, double, bool, Duration, Timestamp, absl::Status, - std::shared_ptr<OtherPropertyValue>>; + std::unique_ptr<OtherPropertyValue>>;Note: This would require updating the wrapper implementations to use
std::moveappropriately.deps/grpc/src/core/channelz/channelz.cc (1)
136-138: Consider making the data collection timeout configurable.The hardcoded 100ms timeout for data collection might be too short in high-load scenarios or too long for low-latency requirements.
Consider making this timeout configurable through channel args or a constant:
+ static constexpr absl::Duration kDataCollectionTimeout = absl::Milliseconds(100); bool completed = - done->WaitForNotificationWithTimeout(absl::Milliseconds(100)); + done->WaitForNotificationWithTimeout(kDataCollectionTimeout);deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc (1)
115-120: Document the magic numbers in delivery data calculation.The hardcoded values (500.0 RTT multiplier and 1e14 fallback rate) lack explanation. Consider adding constants with documentation.
+ // RTT multiplier to account for network variability + static constexpr double kRttMultiplier = 500.0; + // Fallback rate when no rate measurement is available (100 TB/s - effectively unlimited) + static constexpr double kFallbackBytesPerSecond = 1e14; + if (current_rate_ <= 0) { - return DeliveryData{(start_time + rtt_usec_ * 500.0) * 1e-9, 1e14}; + return DeliveryData{(start_time + rtt_usec_ * kRttMultiplier) * 1e-9, kFallbackBytesPerSecond}; } else { - return DeliveryData{(start_time + rtt_usec_ * 500.0) * 1e-9, + return DeliveryData{(start_time + rtt_usec_ * kRttMultiplier) * 1e-9, current_rate_ * 1e9}; }deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc (1)
86-87: Consider graceful handling of unimplemented operations.Using
DCHECKwill crash in debug builds if any unimplemented operation is passed. Consider logging a warning instead.- DCHECK(did_stuff) << "Unimplemented transport perform op "; + if (!did_stuff) { + LOG(WARNING) << "Unimplemented transport perform op"; + }
| CPPFLAGS_asan-noleaks = -O0 -fsanitize-coverage=edge,trace-pc-guard -fsanitize=address -fno-omit-frame-pointer -Wno-unused-command-line-argument | ||
| LDFLAGS_asan-noleaks = fsanitize=address |
There was a problem hiding this comment.
Missing leading dash breaks the linker flag
LDFLAGS_asan-noleaks is defined as
LDFLAGS_asan-noleaks = fsanitize=address
Without the leading - the option is treated as a bare filename, so the build will silently ignore ASan or, worse, attempt to link against a non-existent file named fsanitize=address.
-LDFLAGS_asan-noleaks = fsanitize=address
+LDFLAGS_asan-noleaks = -fsanitize=address📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CPPFLAGS_asan-noleaks = -O0 -fsanitize-coverage=edge,trace-pc-guard -fsanitize=address -fno-omit-frame-pointer -Wno-unused-command-line-argument | |
| LDFLAGS_asan-noleaks = fsanitize=address | |
| CPPFLAGS_asan-noleaks = -O0 -fsanitize-coverage=edge,trace-pc-guard -fsanitize=address -fno-omit-frame-pointer -Wno-unused-command-line-argument | |
| LDFLAGS_asan-noleaks = -fsanitize=address |
🤖 Prompt for AI Agents
In deps/grpc/Makefile at lines 89 to 90, the LDFLAGS_asan-noleaks variable is
missing a leading dash before fsanitize=address, causing the linker to
misinterpret it as a filename. Fix this by adding a leading dash so it reads
-fsanitize=address to correctly pass the address sanitizer flag to the linker.
| static absl::string_view EntityTypeToKind(EntityType type) { | ||
| switch (type) { | ||
| case EntityType::kTopLevelChannel: | ||
| return "channel"; | ||
| case EntityType::kInternalChannel: | ||
| return "internal_channel"; | ||
| case EntityType::kSubchannel: | ||
| return "subchannel"; | ||
| case EntityType::kServer: | ||
| return "server"; | ||
| case EntityType::kListenSocket: | ||
| return "listen_socket"; | ||
| case EntityType::kSocket: | ||
| return "socket"; | ||
| case EntityType::kCall: | ||
| return "call"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add default case or unreachable annotation to EntityTypeToKind.
The switch statement in EntityTypeToKind doesn't have a default case or return statement after the switch, which could cause undefined behavior if the enum is extended.
static absl::string_view EntityTypeToKind(EntityType type) {
switch (type) {
case EntityType::kTopLevelChannel:
return "channel";
case EntityType::kInternalChannel:
return "internal_channel";
case EntityType::kSubchannel:
return "subchannel";
case EntityType::kServer:
return "server";
case EntityType::kListenSocket:
return "listen_socket";
case EntityType::kSocket:
return "socket";
case EntityType::kCall:
return "call";
}
+ GPR_UNREACHABLE_CODE(return "unknown");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static absl::string_view EntityTypeToKind(EntityType type) { | |
| switch (type) { | |
| case EntityType::kTopLevelChannel: | |
| return "channel"; | |
| case EntityType::kInternalChannel: | |
| return "internal_channel"; | |
| case EntityType::kSubchannel: | |
| return "subchannel"; | |
| case EntityType::kServer: | |
| return "server"; | |
| case EntityType::kListenSocket: | |
| return "listen_socket"; | |
| case EntityType::kSocket: | |
| return "socket"; | |
| case EntityType::kCall: | |
| return "call"; | |
| } | |
| } | |
| static absl::string_view EntityTypeToKind(EntityType type) { | |
| switch (type) { | |
| case EntityType::kTopLevelChannel: | |
| return "channel"; | |
| case EntityType::kInternalChannel: | |
| return "internal_channel"; | |
| case EntityType::kSubchannel: | |
| return "subchannel"; | |
| case EntityType::kServer: | |
| return "server"; | |
| case EntityType::kListenSocket: | |
| return "listen_socket"; | |
| case EntityType::kSocket: | |
| return "socket"; | |
| case EntityType::kCall: | |
| return "call"; | |
| } | |
| GPR_UNREACHABLE_CODE(return "unknown"); | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/channelz/channelz.h between lines 130 and 147, the switch
statement in the EntityTypeToKind function lacks a default case or an
unreachable annotation, which risks undefined behavior if the EntityType enum is
extended. Add a default case that either returns a fallback string or marks the
code as unreachable to ensure all enum values are handled safely and the
function always returns a value.
| std::string IntentToStyle(Intent intent) { | ||
| switch (intent) { | ||
| case Intent::kBanner: | ||
| return "zviz-banner"; | ||
| case Intent::kHeading: | ||
| return "zviz-heading"; | ||
| case Intent::kEntityRef: | ||
| return "zviz-entity-ref"; | ||
| case Intent::kTrace: | ||
| return "zviz-trace"; | ||
| case Intent::kTraceDescription: | ||
| return "zviz-trace-description"; | ||
| case Intent::kData: | ||
| return "zviz-data"; | ||
| case Intent::kTimestamp: | ||
| return "zviz-timestamp"; | ||
| case Intent::kNote: | ||
| return "zviz-note"; | ||
| case Intent::kKey: | ||
| return "zviz-key"; | ||
| case Intent::kValue: | ||
| return "zviz-value"; | ||
| case Intent::kDuration: | ||
| return "zviz-duration"; | ||
| } | ||
| } |
There was a problem hiding this comment.
Add default case to prevent undefined behavior
The switch statement in IntentToStyle should include a default case to handle potential future Intent enum values gracefully.
std::string IntentToStyle(Intent intent) {
switch (intent) {
case Intent::kBanner:
return "zviz-banner";
// ... other cases ...
case Intent::kDuration:
return "zviz-duration";
+ default:
+ return "zviz-unknown";
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| std::string IntentToStyle(Intent intent) { | |
| switch (intent) { | |
| case Intent::kBanner: | |
| return "zviz-banner"; | |
| case Intent::kHeading: | |
| return "zviz-heading"; | |
| case Intent::kEntityRef: | |
| return "zviz-entity-ref"; | |
| case Intent::kTrace: | |
| return "zviz-trace"; | |
| case Intent::kTraceDescription: | |
| return "zviz-trace-description"; | |
| case Intent::kData: | |
| return "zviz-data"; | |
| case Intent::kTimestamp: | |
| return "zviz-timestamp"; | |
| case Intent::kNote: | |
| return "zviz-note"; | |
| case Intent::kKey: | |
| return "zviz-key"; | |
| case Intent::kValue: | |
| return "zviz-value"; | |
| case Intent::kDuration: | |
| return "zviz-duration"; | |
| } | |
| } | |
| std::string IntentToStyle(Intent intent) { | |
| switch (intent) { | |
| case Intent::kBanner: | |
| return "zviz-banner"; | |
| case Intent::kHeading: | |
| return "zviz-heading"; | |
| case Intent::kEntityRef: | |
| return "zviz-entity-ref"; | |
| case Intent::kTrace: | |
| return "zviz-trace"; | |
| case Intent::kTraceDescription: | |
| return "zviz-trace-description"; | |
| case Intent::kData: | |
| return "zviz-data"; | |
| case Intent::kTimestamp: | |
| return "zviz-timestamp"; | |
| case Intent::kNote: | |
| return "zviz-note"; | |
| case Intent::kKey: | |
| return "zviz-key"; | |
| case Intent::kValue: | |
| return "zviz-value"; | |
| case Intent::kDuration: | |
| return "zviz-duration"; | |
| default: | |
| return "zviz-unknown"; | |
| } | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/channelz/zviz/layout_html.cc around lines 21 to 46, the
switch statement in the IntentToStyle function lacks a default case, which can
lead to undefined behavior if new Intent enum values are added later. Add a
default case that returns a sensible fallback string or handles unexpected enum
values gracefully to ensure the function always returns a valid string.
| GRPC_CHANNELZ_LOG(channelz_node_) | ||
| << channelz::ChannelNode::GetChannelConnectivityStateChangeString( | ||
| state); | ||
| } else { | ||
| GRPC_CHANNELZ_LOG(channelz_node_) | ||
| << channelz::ChannelNode::GetChannelConnectivityStateChangeString( | ||
| state) | ||
| << " status: " << status.ToString(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the conditional logging logic.
The channelz logging has been updated to use the new macro, but the conditional logic seems inverted: it logs the status only when status.ok() AND state is NOT GRPC_CHANNEL_TRANSIENT_FAILURE. This means error statuses are not logged, which seems counterintuitive for debugging.
Should this condition be inverted to log the status when there's an error?
🏁 Script executed:
#!/bin/bash
# Check other usages of GetChannelConnectivityStateChangeString to understand the pattern
rg -A 5 -B 5 "GetChannelConnectivityStateChangeString" --type cppLength of output: 4447
Invert error-case logging in client_channel_filter.cc
The current logic in client_channel_filter.cc is reversed compared to client_channel.cc: it omits the status on errors but includes it on success. It should mirror the behavior in client_channel.cc, logging the status when !status.ok() or state == GRPC_CHANNEL_TRANSIENT_FAILURE.
Locations to update:
- deps/grpc/src/core/client_channel/client_channel_filter.cc, around lines 1543–1551
Proposed diff:
--- a/deps/grpc/src/core/client_channel/client_channel_filter.cc
+++ b/deps/grpc/src/core/client_channel/client_channel_filter.cc
@@ -1543,8 +1543,8 @@ void ClientChannelFilter::OnConnectivityStateChange(
channelz_node_->SetConnectivityState(state);
- if (!status.ok() || state == GRPC_CHANNEL_TRANSIENT_FAILURE) {
- GRPC_CHANNELZ_LOG(channelz_node_)
- << channelz::ChannelNode::GetChannelConnectivityStateChangeString(
- state);
- } else {
- GRPC_CHANNELZ_LOG(channelz_node_)
- << channelz::ChannelNode::GetChannelConnectivityStateChangeString(
- state)
- << " status: " << status.ToString();
- }
+ if (!status.ok() || state == GRPC_CHANNEL_TRANSIENT_FAILURE) {
+ GRPC_CHANNELZ_LOG(channelz_node_)
+ << channelz::ChannelNode::GetChannelConnectivityStateChangeString(state)
+ << " status: " << status.ToString();
+ } else {
+ GRPC_CHANNELZ_LOG(channelz_node_)
+ << channelz::ChannelNode::GetChannelConnectivityStateChangeString(state);
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GRPC_CHANNELZ_LOG(channelz_node_) | |
| << channelz::ChannelNode::GetChannelConnectivityStateChangeString( | |
| state); | |
| } else { | |
| GRPC_CHANNELZ_LOG(channelz_node_) | |
| << channelz::ChannelNode::GetChannelConnectivityStateChangeString( | |
| state) | |
| << " status: " << status.ToString(); | |
| } | |
| channelz_node_->SetConnectivityState(state); | |
| if (!status.ok() || state == GRPC_CHANNEL_TRANSIENT_FAILURE) { | |
| GRPC_CHANNELZ_LOG(channelz_node_) | |
| << channelz::ChannelNode::GetChannelConnectivityStateChangeString(state) | |
| << " status: " << status.ToString(); | |
| } else { | |
| GRPC_CHANNELZ_LOG(channelz_node_) | |
| << channelz::ChannelNode::GetChannelConnectivityStateChangeString(state); | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/client_channel/client_channel_filter.cc around lines 1543
to 1551, the logging logic is inverted: it currently logs the status only on
success but omits it on errors. Modify the condition to log the status when the
status is not OK or when the state equals GRPC_CHANNEL_TRANSIENT_FAILURE,
mirroring the behavior in client_channel.cc. Adjust the if-else branches
accordingly to include the status in error cases and exclude it on success.
| static void UpdateBlackboard(const ServiceConfig& service_config, | ||
| const Blackboard* old_blackboard, | ||
| Blackboard* blackboard); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Document the UpdateBlackboard method.
This new public static method lacks documentation. Please add comments explaining its purpose, when it should be called, and what it does with the service config and blackboard parameters.
+ // Updates the blackboard with retry throttling configuration from the service config.
+ // If old_blackboard is provided, it may be used to carry forward existing state.
static void UpdateBlackboard(const ServiceConfig& service_config,
const Blackboard* old_blackboard,
Blackboard* blackboard);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static void UpdateBlackboard(const ServiceConfig& service_config, | |
| const Blackboard* old_blackboard, | |
| Blackboard* blackboard); | |
| // Updates the blackboard with retry throttling configuration from the service config. | |
| // If old_blackboard is provided, it may be used to carry forward existing state. | |
| static void UpdateBlackboard(const ServiceConfig& service_config, | |
| const Blackboard* old_blackboard, | |
| Blackboard* blackboard); |
🤖 Prompt for AI Agents
In deps/grpc/src/core/client_channel/retry_filter.h around lines 48 to 51, the
public static method UpdateBlackboard lacks documentation. Add a comment above
the method declaration explaining its purpose, when it should be called, and how
it uses the service_config and blackboard parameters to update the blackboard
state based on the service configuration.
| Http2ErrorCode code = settings_.mutable_local().Apply( | ||
| Http2Settings::kInitialWindowSizeWireId, | ||
| (Http2Settings::max_initial_window_size() - 1)); | ||
| DCHECK(code == Http2ErrorCode::kNoError); | ||
| std::optional<Http2SettingsFrame> settings_frame = |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify error handling for settings application.
The code uses DCHECK for the error code but doesn't handle potential failures in production.
Consider handling the error gracefully:
Http2ErrorCode code = settings_.mutable_local().Apply(
Http2Settings::kInitialWindowSizeWireId,
(Http2Settings::max_initial_window_size() - 1));
- DCHECK(code == Http2ErrorCode::kNoError);
+ if (code != Http2ErrorCode::kNoError) {
+ LOG(ERROR) << "Failed to apply initial window size setting: " << code;
+ // Consider appropriate error handling
+ }🏁 Script executed:
#!/bin/bash
# Check if there are other places where settings are applied and how errors are handled
ast-grep --pattern 'settings_.mutable_local().Apply($$$)'Length of output: 458
Handle failure from Apply when setting initial window size
The call to settings_.mutable_local().Apply(...) at
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc:714–716
is the only occurrence in the repo, and currently uses DCHECK, which is stripped out in release builds. We should add a runtime check to catch and handle failures in production.
• Location to update:
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc lines 714–718
Suggested change:
Http2ErrorCode code = settings_.mutable_local().Apply(
Http2Settings::kInitialWindowSizeWireId,
(Http2Settings::max_initial_window_size() - 1));
- DCHECK(code == Http2ErrorCode::kNoError);
+ if (code != Http2ErrorCode::kNoError) {
+ LOG(ERROR) << "Failed to apply initial window size setting: " << code;
+ // TODO: decide how to recover—e.g., abort transport startup or signal error upstream
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Http2ErrorCode code = settings_.mutable_local().Apply( | |
| Http2Settings::kInitialWindowSizeWireId, | |
| (Http2Settings::max_initial_window_size() - 1)); | |
| DCHECK(code == Http2ErrorCode::kNoError); | |
| std::optional<Http2SettingsFrame> settings_frame = | |
| Http2ErrorCode code = settings_.mutable_local().Apply( | |
| Http2Settings::kInitialWindowSizeWireId, | |
| (Http2Settings::max_initial_window_size() - 1)); | |
| if (code != Http2ErrorCode::kNoError) { | |
| LOG(ERROR) << "Failed to apply initial window size setting: " << code; | |
| // TODO: decide how to recover—e.g., abort transport startup or signal error upstream | |
| } | |
| std::optional<Http2SettingsFrame> settings_frame = |
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
around lines 714 to 718, replace the DCHECK that verifies the success of
settings_.mutable_local().Apply(...) with a runtime check that handles failure
in production builds. After calling Apply, check if the returned code is not
Http2ErrorCode::kNoError, and if so, handle the error appropriately (e.g., log
the error and return or abort) to ensure failures are caught and managed outside
of debug builds.
| // https://datatracker.ietf.org/doc/html/rfc9113#name-stream-identifiers | ||
| // TODO(tjagtap) : [PH2][P0] Validate implementation. | ||
| // TODO(tjagtap) : [PH2][P2] Validate implementation. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add stream ID validation per RFC 9113.
Stream identifiers must be validated according to RFC 9113 requirements.
Add validation before creating the stream:
bool Http2ClientTransport::MakeStream(CallHandler call_handler,
const uint32_t stream_id) {
// https://datatracker.ietf.org/doc/html/rfc9113#name-stream-identifiers
- // TODO(tjagtap) : [PH2][P2] Validate implementation.
+ // Client-initiated streams must use odd stream IDs
+ if ((stream_id & 1) == 0) {
+ LOG(ERROR) << "Invalid stream ID for client: " << stream_id;
+ return false;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // https://datatracker.ietf.org/doc/html/rfc9113#name-stream-identifiers | |
| // TODO(tjagtap) : [PH2][P0] Validate implementation. | |
| // TODO(tjagtap) : [PH2][P2] Validate implementation. | |
| bool Http2ClientTransport::MakeStream(CallHandler call_handler, | |
| const uint32_t stream_id) { | |
| // https://datatracker.ietf.org/doc/html/rfc9113#name-stream-identifiers | |
| - // TODO(tjagtap) : [PH2][P2] Validate implementation. | |
| + // Client-initiated streams must use odd stream IDs | |
| + if ((stream_id & 1) == 0) { | |
| + LOG(ERROR) << "Invalid stream ID for client: " << stream_id; | |
| + return false; | |
| + } | |
| // ... rest of implementation ... |
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
around lines 869 to 870, the code lacks validation of stream identifiers as
required by RFC 9113. Add logic to validate the stream ID before creating the
stream, ensuring it meets the RFC rules such as being a positive odd number for
client-initiated streams and within the allowed range. Implement checks to
reject invalid stream IDs and handle errors appropriately before proceeding with
stream creation.
| // TODO(akshitpatel) : [PH2][P2] : Eventually there should be a separate ref | ||
| // counted struct/class passed to all the transport promises/members. This | ||
| // will help removing back references from the transport members to | ||
| // transport and greatly simpilfy the cleanup path. | ||
| Http2ClientTransport* transport_; | ||
| explicit PingSystemInterfaceImpl(Http2ClientTransport* transport) | ||
| : transport_(transport) {} | ||
| }; |
There was a problem hiding this comment.
Address transport lifetime management in interface implementations.
Both PingSystemInterfaceImpl and KeepAliveInterfaceImpl store raw pointers to the transport, which could become dangling if the transport is destroyed while these interfaces are still in use.
The TODO comments acknowledge this issue. Consider using RefCountedPtr or implementing the suggested separate ref-counted structure to ensure proper lifetime management:
private:
- Http2ClientTransport* transport_;
+ RefCountedPtr<Http2ClientTransport> transport_;
explicit PingSystemInterfaceImpl(Http2ClientTransport* transport)
- : transport_(transport) {}
+ : transport_(transport->RefAsSubclass<Http2ClientTransport>()) {}Also applies to: 575-581
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h
around lines 527 to 534 and 575 to 581, the PingSystemInterfaceImpl and
KeepAliveInterfaceImpl classes hold raw pointers to the transport, risking
dangling pointers if the transport is destroyed prematurely. To fix this,
replace the raw Http2ClientTransport* pointers with RefCountedPtr or a similar
reference-counted smart pointer to manage the transport's lifetime safely.
Alternatively, implement a separate ref-counted structure as suggested in the
TODO to hold shared ownership and prevent premature destruction.
| #include <string> | ||
| #include <variant> | ||
|
|
||
| #include "src/core/channelz/property_list.h" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify necessary includes.
The new includes for channelz/property_list.h and util/string.h should be verified to ensure they are actually needed for the implementation.
Also applies to: 27-27
🏁 Script executed:
#!/bin/bash
# Description: Verify if the new includes are used in the header file
# Check if channelz::PropertyList is used
rg -A 10 -B 5 "channelz::PropertyList" deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.h
# Check if anything from util/string.h is used
rg -A 5 -B 5 "string" deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.hLength of output: 1487
🏁 Script executed:
#!/bin/bash
# Inspect the contents of util/string.h to see what it provides
sed -n '1,200p' deps/grpc/src/core/util/string.hLength of output: 4242
Remove Unused util/string.h Include
The new #include "src/core/util/string.h" isn’t needed—no symbols from that file are used in ping_rate_policy.h. You can safely drop it.
• File: deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.h
– Remove line importing src/core/util/string.h
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.h at line
25, remove the #include directive for "src/core/util/string.h" since no symbols
from that file are used in this header, making the include unnecessary.
| if (grpc_core::IsChttp2BoundWriteSizeEnabled()) { | ||
| if (t_->outbuf.c_slice_buffer()->length >= target_write_size_) { | ||
| result_.partial = true; | ||
| return nullptr; | ||
| } | ||
| } else { | ||
| // TODO(ctiller): this is likely buggy now, but everything seems to be | ||
| // working, so I'm keeping the above fix just for the experiment until | ||
| // we've had time to soak it fully. | ||
| if (t_->outbuf.c_slice_buffer()->length > target_write_size_) { | ||
| result_.partial = true; | ||
| return nullptr; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the write size boundary logic change
The new experimental flag introduces a subtle but important change from > to >= in the boundary check. The comment indicates uncertainty about the old logic being "likely buggy". This warrants verification to ensure the change doesn't introduce regressions.
Please also verify through testing that this change doesn't affect write performance or cause any edge cases with exact boundary conditions.
🏁 Script executed:
#!/bin/bash
# Check for any tests or documentation about the write size boundary behavior
rg -A 5 -B 5 "target_write_size|bound.*write.*size" --type cc --type h | grep -E "(test|TEST|Test)" | head -20
# Look for the experimental flag definition
ast-grep --pattern 'IsChttp2BoundWriteSizeEnabled()'Length of output: 194
Please review the output to confirm:
- Whether any tests or docs mention the exact boundary behavior.
- Where and how the feature flag is defined (e.g., env var, default value).
- That no other code paths depend on the old
>check.
If these references are missing or inconclusive, please add/verify tests around the >= versus > behavior and document the flag’s intended semantics.
🏁 Script executed:
#!/bin/bash
# Search for any references to target_write_size_ in the C++ sources
rg -n "target_write_size_" .
# Find all usages of the experimental flag
rg -n "IsChttp2BoundWriteSizeEnabled" .
# Look for where the flag is defined or documented
rg -n "BoundWriteSizeEnabled" .Length of output: 2539
Ensure accurate boundary checks and add tests/docs for the experimental flag
The default behavior (IsChttp2BoundWriteSizeEnabled() == false) remains unchanged (>), so there’s no immediate regression. However, there are no existing tests or documentation covering the new >= behavior when the flag is enabled. Please:
• Add unit tests in transport/chttp2 to verify that:
- With the flag disabled, writes only become partial when
length > target_write_size_. - With the flag enabled, writes become partial at
length >= target_write_size_.
• Update the experiments documentation (in deps/grpc/src/core/lib/experiments/experiments.h or related docs) to describe the semantics of IsChttp2BoundWriteSizeEnabled.
• (Optional) Smoke-test performance impact with the flag enabled to rule out regressions.
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc around lines 344
to 357, the boundary check behavior differs based on the
IsChttp2BoundWriteSizeEnabled flag, but there are no tests or documentation for
the new >= condition when enabled. Add unit tests in transport/chttp2 to verify
that with the flag disabled, partial writes occur only when length >
target_write_size_, and with the flag enabled, partial writes occur at length >=
target_write_size_. Also, update the experiments documentation in
deps/grpc/src/core/lib/experiments/experiments.h or related files to clearly
describe the semantics of IsChttp2BoundWriteSizeEnabled. Optionally, perform a
smoke test to check for any performance regressions with the flag enabled.
santigimeno
force-pushed
the
santi/grpc_1.74.0
branch
from
6891c52 to
6c2126f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h (1)
307-323: Implement HTTP/2 stream ID exhaustion handlingThe call to
NextStreamId()in
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h(lines 307–323) checks againstRFC9113::kMaxStreamId31Bit(0x7FFFFFFF, defined inframe.h) but currently only leaves a TODO and still returns an invalid stream ID when exhausted. Per RFC9113 §5.1.1, a client that cannot establish a new stream identifier must send a GOAWAY on the existing connection and open a new connection for subsequent streams.Please update this code to:
- Detect
*next_stream_id_lock > RFC9113::kMaxStreamId31Bit- Send a GOAWAY frame on the current transport
- Drain or close the exhausted transport
- Initialize a new transport for future streams
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc (1)
384-392: Missing window update processing will break flow control.Without processing WINDOW_UPDATE frames, the send window will never be replenished, causing the connection to stall once the initial window is exhausted.
This must be implemented before the transport can function correctly. At minimum:
GRPC_HTTP2_CLIENT_DLOG << "Http2Transport ProcessHttp2WindowUpdateFrame Promise { " " stream_id=" << frame.stream_id << ", increment=" << frame.increment << "}"; + // Critical: Update the send window for the stream or connection + if (frame.stream_id == 0) { + // Update connection-level window + LOG(ERROR) << "Connection window update not yet implemented"; + } else { + // Update stream-level window + LOG(ERROR) << "Stream window update not yet implemented for stream " << frame.stream_id; + } return Http2Status::Ok();
♻️ Duplicate comments (19)
deps/grpc/Makefile (1)
89-90: Missing leading dash inLDFLAGS_asan-noleaksis still unfixedThe flag is emitted as
fsanitize=address; without the leading-the linker treats it as a filename and silently ignores ASan, defeating the purpose of the config.-LDFLAGS_asan-noleaks = fsanitize=address +LDFLAGS_asan-noleaks = -fsanitize=addressdeps/grpc/src/core/ext/transport/chttp2/transport/writing.cc (1)
344-357: Ensure accurate boundary checks and add tests/docs for the experimental flagdeps/grpc/src/core/channelz/zviz/layout_html.cc (1)
21-46: Add default case to prevent undefined behaviordeps/grpc/src/core/client_channel/retry_throttle.h (2)
35-44: Make constructor private to enforce factory pattern.The comment says "Do not instantiate directly -- use Create() instead" but the constructor is public. Make it private and add
Createas a friend function if needed.
60-62: Document the replacement mechanism methods.The
SetReplacementandGetReplacementThrottleDataIfNeededmethods lack documentation. Their purpose and thread-safety guarantees should be clearly documented.deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc (2)
223-273: Consider softer error handling in FdHandler::Handle.The
BindExternalimplementation is well-structured, but the use ofCHECKat line 242 will crash the process ifHandleExternalConnectionfails. Consider using proper error logging and graceful handling instead.
659-676: Document and track Python experimental Event Engine flag.The new
EventEngineExperimentDisabledForPython()guard disables the experimental Event Engine when used from Python. Ensure this behavior is documented and formally tracked.deps/grpc/src/core/ext/transport/chttp2/transport/header_assembler.h (3)
99-101: Consider using runtime validation instead of DCHECK for stream ID.
134-147: Add validation checks for CONTINUATION frames.
165-169: Implement proper metadata size limits.deps/grpc/src/core/channelz/channelz.h (1)
130-147: Add default case or unreachable annotation to EntityTypeToKind.The switch statement in
EntityTypeToKinddoesn't have a default case or return statement after the switch, which could cause undefined behavior if the enum is extended.deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h (1)
117-127: Ensure proper synchronization for scheduling state cleanup.The destructor reads
scheduling_state_with acquire ordering but the state transitions might need stronger ordering to ensure the Waker pointer is visible.deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h (2)
527-534: Address transport lifetime management in interface implementations.The
PingSystemInterfaceImplstores a raw pointer to the transport, which could become dangling if the transport is destroyed while the interface is still in use.
575-581: Address transport lifetime management in interface implementations.The
KeepAliveInterfaceImplstores a raw pointer to the transport, which could become dangling if the transport is destroyed while the interface is still in use.deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc (1)
247-306: LOG(FATAL) usage remains unaddressed.The LOG(FATAL) statements at lines 274-275 and 304-305 that were identified in previous reviews are still present. These should be replaced with safer alternatives to avoid crashing the process in production.
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc (4)
217-229: Incomplete stream validation may cause protocol violations.The header frame processing returns
Ok()when stream lookup fails, but RFC 9113 requires a PROTOCOL_ERROR for unexpected stream identifiers.
371-379: GOAWAY frame processing is not implemented.The GOAWAY frame handler returns
Ok()without processing, which could cause the client to continue using a connection that the server is shutting down.
714-718: Handle failure from Apply when setting initial window size.The
DCHECKonly works in debug builds. Production builds need runtime error handling.
869-870: Add stream ID validation per RFC 9113.Client-initiated streams must use odd stream IDs.
🧹 Nitpick comments (3)
deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h (1)
141-155: Consider graceful handling if Drop() is not called.The destructor's
CHECK(dropped_)will crash ifDrop()is not called. While good for development, consider logging an error and callingDrop()in the destructor for production builds.- ~Reader() { CHECK(dropped_); } + ~Reader() { + if (!dropped_) { + LOG(ERROR) << "Reader destroyed without calling Drop()"; +#ifdef DEBUG + CHECK(dropped_); +#else + Drop(); +#endif + } + }deps/grpc/src/core/channelz/channelz.cc (1)
125-139: Consider making the data collection timeout configurable.The hardcoded 100ms timeout might not be suitable for all environments. Consider making it configurable via channel args or a constant.
+ static constexpr absl::Duration kDataCollectionTimeout = absl::Milliseconds(100); bool completed = - done->WaitForNotificationWithTimeout(absl::Milliseconds(100)); + done->WaitForNotificationWithTimeout(kDataCollectionTimeout);deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc (1)
415-415: Fix typo in variable name.- HeaderAssembler& assember = stream->header_assembler; + HeaderAssembler& assembler = stream->header_assembler;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (154)
deps/grpc/.bazelci/presubmit.yml(1 hunks)deps/grpc/Makefile(22 hunks)deps/grpc/grpc.gyp(11 hunks)deps/grpc/include/grpc/create_channel_from_endpoint.h(1 hunks)deps/grpc/include/grpc/credentials.h(1 hunks)deps/grpc/include/grpc/event_engine/event_engine.h(7 hunks)deps/grpc/include/grpc/grpc_posix.h(1 hunks)deps/grpc/include/grpc/impl/channel_arg_names.h(1 hunks)deps/grpc/include/grpc/module.modulemap(1 hunks)deps/grpc/include/grpc/support/json.h(1 hunks)deps/grpc/include/grpcpp/channel.h(2 hunks)deps/grpc/include/grpcpp/create_channel_posix.h(2 hunks)deps/grpc/include/grpcpp/impl/generic_serialize.h(3 hunks)deps/grpc/include/grpcpp/security/credentials.h(2 hunks)deps/grpc/include/grpcpp/support/callback_common.h(2 hunks)deps/grpc/include/grpcpp/support/client_callback.h(3 hunks)deps/grpc/include/grpcpp/version_info.h(1 hunks)deps/grpc/src/compiler/php_generator.cc(1 hunks)deps/grpc/src/core/BUILD(96 hunks)deps/grpc/src/core/call/filter_fusion.h(2 hunks)deps/grpc/src/core/call/interception_chain.h(4 hunks)deps/grpc/src/core/channelz/channel_trace.cc(1 hunks)deps/grpc/src/core/channelz/channel_trace.h(3 hunks)deps/grpc/src/core/channelz/channelz.cc(20 hunks)deps/grpc/src/core/channelz/channelz.h(20 hunks)deps/grpc/src/core/channelz/channelz_registry.cc(1 hunks)deps/grpc/src/core/channelz/channelz_registry.h(3 hunks)deps/grpc/src/core/channelz/property_list.cc(1 hunks)deps/grpc/src/core/channelz/property_list.h(1 hunks)deps/grpc/src/core/channelz/ztrace_collector.h(3 hunks)deps/grpc/src/core/channelz/zviz/data.cc(1 hunks)deps/grpc/src/core/channelz/zviz/data.h(1 hunks)deps/grpc/src/core/channelz/zviz/entity.cc(1 hunks)deps/grpc/src/core/channelz/zviz/entity.h(1 hunks)deps/grpc/src/core/channelz/zviz/environment.cc(1 hunks)deps/grpc/src/core/channelz/zviz/environment.h(1 hunks)deps/grpc/src/core/channelz/zviz/html.cc(1 hunks)deps/grpc/src/core/channelz/zviz/html.h(1 hunks)deps/grpc/src/core/channelz/zviz/layout.cc(1 hunks)deps/grpc/src/core/channelz/zviz/layout.h(1 hunks)deps/grpc/src/core/channelz/zviz/layout_html.cc(1 hunks)deps/grpc/src/core/channelz/zviz/layout_html.h(1 hunks)deps/grpc/src/core/channelz/zviz/strings.cc(1 hunks)deps/grpc/src/core/channelz/zviz/strings.h(1 hunks)deps/grpc/src/core/channelz/zviz/trace.cc(1 hunks)deps/grpc/src/core/channelz/zviz/trace.h(1 hunks)deps/grpc/src/core/client_channel/client_channel.cc(5 hunks)deps/grpc/src/core/client_channel/client_channel_filter.cc(4 hunks)deps/grpc/src/core/client_channel/config_selector.h(2 hunks)deps/grpc/src/core/client_channel/dynamic_filters.cc(2 hunks)deps/grpc/src/core/client_channel/dynamic_filters.h(1 hunks)deps/grpc/src/core/client_channel/global_subchannel_pool.cc(1 hunks)deps/grpc/src/core/client_channel/retry_filter.cc(1 hunks)deps/grpc/src/core/client_channel/retry_filter.h(4 hunks)deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.cc(3 hunks)deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.h(1 hunks)deps/grpc/src/core/client_channel/retry_interceptor.cc(5 hunks)deps/grpc/src/core/client_channel/retry_interceptor.h(3 hunks)deps/grpc/src/core/client_channel/retry_throttle.cc(3 hunks)deps/grpc/src/core/client_channel/retry_throttle.h(1 hunks)deps/grpc/src/core/client_channel/subchannel.cc(6 hunks)deps/grpc/src/core/client_channel/subchannel.h(2 hunks)deps/grpc/src/core/config/config_vars.cc(1 hunks)deps/grpc/src/core/config/config_vars.yaml(1 hunks)deps/grpc/src/core/config/core_configuration.cc(1 hunks)deps/grpc/src/core/config/core_configuration.h(5 hunks)deps/grpc/src/core/credentials/call/call_creds_registry.h(1 hunks)deps/grpc/src/core/credentials/call/call_creds_registry_init.cc(1 hunks)deps/grpc/src/core/credentials/call/gcp_service_account_identity/gcp_service_account_identity_credentials.cc(2 hunks)deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.cc(1 hunks)deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.h(1 hunks)deps/grpc/src/core/credentials/call/jwt_util.cc(1 hunks)deps/grpc/src/core/credentials/call/jwt_util.h(1 hunks)deps/grpc/src/core/credentials/transport/channel_creds_registry_init.cc(1 hunks)deps/grpc/src/core/credentials/transport/google_default/google_default_credentials.cc(4 hunks)deps/grpc/src/core/credentials/transport/ssl/ssl_credentials.cc(0 hunks)deps/grpc/src/core/credentials/transport/tls/load_system_roots_supported.cc(1 hunks)deps/grpc/src/core/credentials/transport/xds/xds_credentials.cc(0 hunks)deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc(1 hunks)deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h(1 hunks)deps/grpc/src/core/ext/filters/http/client_authority_filter.cc(1 hunks)deps/grpc/src/core/ext/filters/http/message_compress/compression_filter.h(4 hunks)deps/grpc/src/core/ext/filters/http/server/http_server_filter.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/client_transport.cc(6 hunks)deps/grpc/src/core/ext/transport/chaotic_good/client_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/config.h(4 hunks)deps/grpc/src/core/ext/transport/chaotic_good/control_endpoint.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc(6 hunks)deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h(5 hunks)deps/grpc/src/core/ext/transport/chaotic_good/frame.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/frame_transport.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/message_chunker.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/scheduler.cc(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/scheduler.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc(11 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h(5 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server_transport.cc(5 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_header.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.cc(7 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_ztrace_collector.h(3 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/chaotic_good_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/client_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/server/chaotic_good_server.cc(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.cc(4 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/client/chttp2_connector.cc(7 hunks)deps/grpc/src/core/ext/transport/chttp2/server/chttp2_server.cc(4 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.cc(17 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.h(0 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/flow_control.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame_data.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame_settings.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame_window_update.cc(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/header_assembler.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc(5 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.h(4 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc(15 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h(11 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.cc(10 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_status.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_transport.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_ztrace_collector.h(0 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/internal.h(6 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.cc(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.h(4 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/message_assembler.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/parsing.cc(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_callbacks.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.cc(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.h(0 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/stream_data_queue.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/stream_lists.cc(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc(10 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.c(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.c(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/property_list.upb.h(1 hunks)
💤 Files with no reviewable changes (5)
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.h
- deps/grpc/src/core/credentials/transport/ssl/ssl_credentials.cc
- deps/grpc/src/core/credentials/transport/xds/xds_credentials.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_ztrace_collector.h
✅ Files skipped from review due to trivial changes (19)
- deps/grpc/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc
- deps/grpc/.bazelci/presubmit.yml
- deps/grpc/src/core/config/config_vars.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/stream_data_queue.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.h
- deps/grpc/src/core/credentials/call/jwt_util.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_transport.h
- deps/grpc/include/grpcpp/version_info.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.h
- deps/grpc/include/grpc/impl/channel_arg_names.h
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/client_transport.h
- deps/grpc/src/core/ext/transport/chaotic_good/client_transport.h
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/chaotic_good_transport.h
- deps/grpc/src/core/credentials/transport/channel_creds_registry_init.cc
- deps/grpc/include/grpcpp/support/client_callback.h
- deps/grpc/include/grpcpp/security/credentials.h
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.cc
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.h
🚧 Files skipped from review as they are similar to previous changes (102)
- deps/grpc/include/grpc/module.modulemap
- deps/grpc/src/core/credentials/transport/tls/load_system_roots_supported.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/frame_data.cc
- deps/grpc/src/core/ext/filters/http/client_authority_filter.cc
- deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_header.h
- deps/grpc/src/core/ext/transport/chaotic_good/frame_transport.h
- deps/grpc/src/compiler/php_generator.cc
- deps/grpc/src/core/channelz/ztrace_collector.h
- deps/grpc/src/core/channelz/channelz_registry.cc
- deps/grpc/include/grpcpp/channel.h
- deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h
- deps/grpc/src/core/ext/transport/chaotic_good/control_endpoint.h
- deps/grpc/src/core/client_channel/global_subchannel_pool.cc
- deps/grpc/include/grpc/support/json.h
- deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.h
- deps/grpc/src/core/ext/transport/chttp2/transport/frame.h
- deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc
- deps/grpc/src/core/channelz/zviz/strings.h
- deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.h
- deps/grpc/include/grpc/grpc_posix.h
- deps/grpc/src/core/config/config_vars.yaml
- deps/grpc/include/grpc/create_channel_from_endpoint.h
- deps/grpc/src/core/credentials/call/gcp_service_account_identity/gcp_service_account_identity_credentials.cc
- deps/grpc/src/core/call/interception_chain.h
- deps/grpc/src/core/channelz/zviz/environment.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.cc
- deps/grpc/src/core/config/core_configuration.cc
- deps/grpc/src/core/ext/transport/chaotic_good/frame.h
- deps/grpc/include/grpc/credentials.h
- deps/grpc/src/core/ext/transport/chaotic_good/server_transport.h
- deps/grpc/src/core/ext/transport/chttp2/transport/frame.cc
- deps/grpc/src/core/channelz/zviz/data.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_callbacks.h
- deps/grpc/src/core/channelz/zviz/entity.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.cc
- deps/grpc/src/core/client_channel/subchannel.h
- deps/grpc/src/core/ext/transport/chaotic_good/config.h
- deps/grpc/src/core/channelz/zviz/trace.h
- deps/grpc/src/core/channelz/zviz/strings.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/parsing.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.h
- deps/grpc/src/core/client_channel/dynamic_filters.h
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/server/chaotic_good_server.cc
- deps/grpc/src/core/ext/transport/chaotic_good/message_chunker.h
- deps/grpc/src/core/channelz/zviz/trace.cc
- deps/grpc/src/core/ext/transport/chaotic_good/server_transport.cc
- deps/grpc/include/grpcpp/impl/generic_serialize.h
- deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/flow_control.h
- deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.cc
- deps/grpc/include/grpcpp/support/callback_common.h
- deps/grpc/src/core/ext/transport/chttp2/transport/frame_window_update.cc
- deps/grpc/src/core/channelz/zviz/entity.cc
- deps/grpc/src/core/ext/transport/chttp2/server/chttp2_server.cc
- deps/grpc/src/core/config/core_configuration.h
- deps/grpc/src/core/ext/transport/chttp2/transport/stream_lists.cc
- deps/grpc/src/core/ext/filters/http/server/http_server_filter.h
- deps/grpc/src/core/credentials/call/jwt_util.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_status.h
- deps/grpc/src/core/channelz/zviz/layout.cc
- deps/grpc/src/core/client_channel/config_selector.h
- deps/grpc/include/grpcpp/create_channel_posix.h
- deps/grpc/src/core/ext/transport/chttp2/transport/frame_settings.cc
- deps/grpc/src/core/call/filter_fusion.h
- deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.h
- deps/grpc/src/core/channelz/zviz/layout_html.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings.h
- deps/grpc/src/core/channelz/zviz/environment.h
- deps/grpc/src/core/channelz/zviz/data.cc
- deps/grpc/grpc.gyp
- deps/grpc/src/core/client_channel/client_channel_filter.cc
- deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h
- deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.h
- deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.h
- deps/grpc/src/core/credentials/call/call_creds_registry_init.cc
- deps/grpc/src/core/ext/transport/chttp2/client/chttp2_connector.cc
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.c
- deps/grpc/src/core/ext/transport/chaotic_good/scheduler.h
- deps/grpc/src/core/client_channel/retry_interceptor.h
- deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.h
- deps/grpc/src/core/client_channel/dynamic_filters.cc
- deps/grpc/src/core/ext/filters/http/message_compress/compression_filter.h
- deps/grpc/src/core/channelz/zviz/html.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/internal.h
- deps/grpc/src/core/client_channel/retry_filter.h
- deps/grpc/src/core/ext/transport/chttp2/transport/message_assembler.h
- deps/grpc/src/core/channelz/zviz/html.h
- deps/grpc/src/core/channelz/channelz_registry.h
- deps/grpc/src/core/ext/transport/chaotic_good/scheduler.cc
- deps/grpc/src/core/channelz/zviz/layout.h
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/property_list.upb.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.c
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb.h
- deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional javascript-facing argume...
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
Applied to files:
deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc
📚 Learning: the `span` class in `doc/api/diagnostics_channel.md` is unrelated to the one in `lib/internal/otel/t...
Learnt from: santigimeno
PR: nodesource/nsolid#289
File: lib/internal/otel/trace.js:40-40
Timestamp: 2025-04-16T06:58:59.313Z
Learning: The `Span` class in `doc/api/diagnostics_channel.md` is unrelated to the one in `lib/internal/otel/trace.js`. They share the same name but serve completely different purposes - one is for documentation examples, while the other is part of the OpenTelemetry implementation.
Applied to files:
deps/grpc/src/core/channelz/channel_trace.h
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: build-docs
- GitHub Check: build-tarball
- GitHub Check: test-macOS
- GitHub Check: lint-js-and-md
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-linux (ubuntu-24.04-arm)
🔇 Additional comments (80)
deps/grpc/src/core/client_channel/retry_filter.cc (2)
95-112: LGTM!The blackboard update logic correctly migrates the retry throttler state while updating it with new configuration parameters.
114-121: LGTM!The constructor correctly initializes all members using the new blackboard-based approach.
deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc (2)
142-142: Good improvement: Transport-specific statisticsThe switch from global to transport-specific statistics improves observability by allowing per-transport metrics collection.
Also applies to: 263-264, 288-288, 557-557, 564-564
376-381: Well-designed TCP tracer integrationThe implementation properly maintains backward compatibility while introducing the new event engine-based TCP tracing mechanism. The conditional logic ensures the appropriate method is used based on the endpoint type.
Also applies to: 733-742
deps/grpc/src/core/credentials/transport/google_default/google_default_credentials.cc (2)
346-399: Well-designed dual credentials wrapperThe
GoogleDefaultCallCredentialsWrappercorrectly implements dynamic credential selection based on the transport security type, providing seamless TLS/ALTS switching.
435-480: Correct implementation of dual credential supportThe function properly handles both TLS-only and dual TLS/ALTS credential scenarios, maintaining backward compatibility while enabling the new dual credential functionality.
deps/grpc/src/core/client_channel/client_channel.cc (2)
498-501: Good simplification of channelz loggingThe switch to
GRPC_CHANNELZ_LOGmacro eliminates boilerplate and makes the logging code more maintainable.Also applies to: 1150-1151, 1309-1316
1266-1285: Correct blackboard state migrationThe updated blackboard handling properly migrates retry throttling state and other configuration between old and new blackboards, ensuring continuity during service config updates.
deps/grpc/src/core/client_channel/retry_throttle.cc (1)
49-77: LGTM!The factory pattern implementation is well-designed with proper reuse of existing throttlers when parameters match, and the token scaling logic for transitioning between old and new throttlers is correctly implemented.
deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc (1)
83-104: Well-structured logging helpers.The new
LogInitFailureandLogInformationalfunctions provide consistent error reporting with proper channelz integration.deps/grpc/src/core/ext/transport/chaotic_good/client_transport.cc (2)
246-248: Good defensive programming with assertion.Using
DCHECKto assertparty_is null in the destructor helps catch lifecycle bugs during development.
237-237: Clarify the huge MPSC buffer capacityThe ChaoticGood client transport now constructs its outgoing‐frame queue with
MpscReceiver<OutgoingFrame> outgoing_frames{256 * 1024 * 1024};(268 435 456 entries), up from the legacy value of 4—and the CHTTP2 transports default to just 10. Allocating hundreds of millions of slots per connection can consume gigabytes of RAM.
• File: deps/grpc/src/core/ext/transport/chaotic_good/client_transport.cc (≈line 237)
• Note: the server counterpart in deps/grpc/src/core/ext/transport/chaotic_good/server_transport.cc also uses 256 × 10⁶ entriesPlease confirm this capacity is intentional and add a comment or documentation explaining:
- Why such a large fixed size is needed
- Whether it should be configurable via ChannelArgs or a build‐time flag
deps/grpc/src/core/client_channel/subchannel.cc (3)
425-439: Correct handling of endpoint-created subchannels.The logic to keep endpoint-created subchannels in
TRANSIENT_FAILUREstate instead of transitioning toIDLEis appropriate since these subchannels cannot establish new connections.
788-797: Proper retry prevention for endpoint subchannels.The early return prevents retry timer setup for endpoint-created subchannels, which is correct since retries are not possible without the ability to create new connections.
588-596: Appropriate bypass of subchannel pool.Skipping the subchannel pool for endpoint-created subchannels and immediately requesting a connection is the right approach for this special case.
deps/grpc/src/core/client_channel/retry_interceptor.cc (1)
137-141: LGTM!The refactoring to use RetryThrottler and integration with the blackboard pattern is well-implemented.
deps/grpc/src/core/channelz/channel_trace.cc (1)
77-91: Well-designed memory-aware trace managementThe implementation correctly handles memory limits by dropping oldest entries and includes an optimization to shrink the vector when needed. The use of EntryRef with salt values for safe referencing is a good design choice.
deps/grpc/include/grpc/event_engine/event_engine.h (2)
316-331: LGTM! Proper move semantics implementationThe move constructor and assignment operator correctly handle the google_specific_ pointer by nullifying the source, preventing potential double-deletion issues.
310-312: WriteArgs destructor implementation confirmedThe
WriteArgs::~WriteArgs()destructor is defined in
deps/grpc/src/core/lib/event_engine/event_engine.ccand correctly deletes thegoogle_specific_pointer when non-null.No further action required.
deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb.h (1)
1-1273: LGTM! Auto-generated protobuf header file.This is a properly structured auto-generated upb protobuf header file for the channelz v2 Promise message types. The code follows standard upb patterns for message creation, parsing, serialization, and field access.
deps/grpc/src/core/ext/transport/chaotic_good/tcp_ztrace_collector.h (4)
75-92: LGTM! Enhanced endpoint metrics tracing.The addition of
endpoint_idfield improves traceability by associating write metrics with specific endpoints.
94-109: LGTM! Well-structured channel scheduling trace.The new
TraceScheduledChannelstruct provides comprehensive scheduling information with clear JSON serialization.
111-133: Verify the retention of MemoryUsage() method.While other trace structs have had their
MemoryUsage()methods removed in this refactoring,TraceWriteScheduleretains it. Is this intentional due to the vector member requiring explicit memory tracking, or should it be removed for consistency?
135-147: LGTM! Refactored large frame header trace.The redefined
WriteLargeFrameHeaderTracestruct provides clear tracing for large frame writes with payload information and endpoint selection.deps/grpc/src/core/channelz/channel_trace.h (5)
54-101: Excellent design for deferred rendering!The
Rendererabstraction with template-based concatenation support provides efficient deferred string rendering. TheAdaptForStoragefunction correctly handles pointer lifetime issues by convertingconst char*tostring_view.
115-143: LGTM! Clever streaming log implementation.The
LogExprtemplate provides elegant streaming syntax with automatic commit on destruction. The move semantics and nullptr guards ensure safe single-ownership.
198-268: Excellent RAII design for trace nodes!The
Nodeclass provides robust RAII semantics with automatic cleanup of uncommitted entries. The move-only design ensures clear ownership, and the hierarchical parent-child support enables structured tracing.
305-348: Well-designed compact entry structure.The
Entrystruct efficiently uses 16-bit indices for memory compactness. Note that this limits the maximum number of active entries (including free-list) to 65535. The salt-based validation and dual linking structure (tree + chronological) provide robust traversal capabilities.
433-461: LGTM! Clever macro design with excellent documentation.The
GRPC_CHANNELZ_LOGmacro provides intuitive streaming syntax while avoiding the classic if-else binding security issue through clever use of a for loop. The extensive comment clearly explains the design rationale.deps/grpc/src/core/channelz/property_list.h (3)
36-113: Excellent type-safe property value system!The
PropertyValuevariant and wrapper templates provide comprehensive type support with automatic conversions. The special handling for pointers, optionals, and integral types ensures type safety and prevents null pointer issues.
121-143: LGTM! Clean and flexible PropertyList API.The
PropertyListclass provides an intuitive API with automatic type conversion through the templatedSetmethod. Support for both JSON and protobuf serialization ensures compatibility with different channelz consumers.
148-198: Well-designed 2D property containers!
PropertyGridandPropertyTableprovide flexible 2D property storage with string-based and numeric row indexing respectively. The bulk SetColumn/SetRow operations and ordered tracking ensure efficient and consistent data management.deps/grpc/src/core/channelz/property_list.cc (4)
28-116: LGTM! Comprehensive serialization helpers.The helper functions provide robust conversion between PropertyValue variants and their JSON/protobuf representations. The timestamp and duration handling correctly uses appropriate clock types.
119-161: Solid PropertyList implementation!The implementation correctly handles insertion, deletion, and merging of properties. The protobuf Any serialization properly sets the type URL for correct deserialization.
163-271: Excellent PropertyGrid implementation!The 2D grid management is well-implemented with proper handling of sparse data (empty cells as protobuf Empty). The key sorting in SetColumn/SetRow ensures consistent ordering.
273-355: LGTM! Consistent PropertyTable implementation.The implementation correctly manages numeric row indices with automatic size tracking. The sparse data handling and protobuf serialization are consistent with PropertyGrid.
deps/grpc/src/core/channelz/channelz.h (7)
149-158: LGTM!The
KindToEntityTypefunction properly handles reverse mapping with appropriate use ofstd::optionalfor error cases.
214-220: LGTM!Centralizing trace management in
BaseNodewith proper accessor methods improves encapsulation and reduces code duplication across derived classes.
283-306: LGTM!The
DataSinkImplementationclass provides a well-designed, thread-safe abstraction for collecting additional node data with support for both JSON and protobuf serialization.
328-343: Well-designed template pattern with clear requirements.The templated
AddDatamethod elegantly handles types that provide bothTakeJsonObject()andFillAny()methods, enabling seamless JSON and protobuf serialization.
235-284: Well-structured stream state machine.The stream state transitions are clearly documented and properly enforced with assertions, ensuring valid state changes throughout the stream lifecycle.
546-560: Correct use of weak references to prevent cycles.Converting child references from
RefCountedPtrtoWeakRefCountedPtrproperly prevents ownership cycles between parent and child nodes.
161-165: All BaseNode subclasses invoke NodeConstructed()
Verified that every class deriving from BaseNode in the channelz code callsNodeConstructed()in its constructor. No further action required.deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h (2)
271-286: LGTM!The
Completionstruct properly manages asynchronous read completion with thread-safe state management and reference counting.
459-462: LGTM!The destructor properly cleans up resources and records endpoint closure in the trace collector.
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h (3)
235-284: Well-designed stream state machine.The stream state transitions are clearly documented with proper DCHECK assertions to catch invalid state changes during development.
358-386: LGTM!The endpoint read methods correctly integrate with keepalive management, with clear reasoning documented for always calling
GotData()regardless of read success.
584-588: Good practice for monitoring class size.The size check helps prevent unintended class bloat and maintains parity with the CHTTP2 implementation.
deps/grpc/src/core/channelz/channelz.cc (4)
63-90: LGTM!The
DataSinkImplementationmethods provide thread-safe data collection with proper support for both JSON and protobuf serialization, including timeout tracking.
96-110: LGTM!The BaseNode lifecycle methods properly manage registration with ChannelzRegistry, with appropriate assertions to catch initialization errors.
241-260: LGTM!The DataSource lifecycle methods use an efficient swap-and-pop pattern for removal with appropriate error logging to catch initialization bugs.
298-307: LGTM!The
CallCounts::ToPropertyList()method properly converts all fields including optional timestamp handling.deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc (10)
17-51: LGTM on include additions.The new includes appropriately support the expanded functionality including channelz property lists, scheduling, telemetry collection, and other core improvements.
65-132: LGTM on SendRate implementation.Good error handling with appropriate validation for negative and NaN values in network metrics. The delivery data calculation logic is mathematically sound and the channelz integration provides good observability.
137-208: LGTM on OutputBuffers::Reader implementation.The asynchronous reading pattern with proper cancellation handling is well implemented. Mutex usage is appropriate and the waker pattern follows gRPC conventions correctly.
308-399: LGTM on sophisticated scheduling implementation.The multi-phase scheduling algorithm is well-structured with proper data collection, planning, and message placement. Good use of latency tracing and appropriate cleanup when readers are unavailable.
416-440: LGTM on SecureFrameQueue implementation.Proper frame serialization with correct padding calculations and alignment handling. Mutex protection and waker pattern are implemented correctly.
445-541: LGTM on InputQueue implementation.Excellent use of bit sets for duplicate detection. Completion handling with proper mutex protection is correct, and the shutdown sequence in SetClosed is comprehensive and graceful.
568-691: LGTM on comprehensive telemetry integration.Excellent integration with event engine telemetry system. Proper handling of various network metrics and well-structured event sink creation for observability.
694-858: LGTM on Endpoint method implementations.Well-structured frame serialization with proper padding and alignment handling. The read/write loops are correctly implemented with appropriate error handling and comprehensive latency tracing.
868-981: LGTM on Endpoint initialization and party setup.Comprehensive initialization of EndpointContext with proper party-based execution model. Good channelz integration and appropriate configuration of endpoint features including tracing and security frames.
988-1021: LGTM on DataEndpoints implementation.Proper initialization of output buffers and input queues with good channelz integration. Resource management is sound and the data collection provides comprehensive observability.
deps/grpc/src/core/BUILD (9)
57-60: LGTM: New config setting follows established patternThe
minimize_threadynessconfig setting is properly added following the same pattern as the existingmaximize_threadynesssetting.
92-107: LGTM: Well-structured header-only libraryThe
memory_usagelibrary is properly defined with appropriate dependencies for status handling and time operations.
556-557: Good: Standardized UPB dependency pathsThe update from colon-separated to slash-separated format for UPB dependencies is correct and follows Bazel conventions.
6537-6562: Excellent: Flexible implementation selection for SharedBitGenThe addition of config settings for CPU-intensive vs memory-intensive implementations with conditional compilation is a well-designed approach for optimizing based on deployment requirements.
10193-10215: LGTM: Channelz property list library properly configuredThe new
channelz_property_listlibrary is well-structured with appropriate dependencies for protobuf and JSON handling, supporting the channelz v2 enhancements.
10217-10260: Good addition: Comprehensive SSL transport security utilitiesThe
ssl_transport_security_utilslibrary provides essential SSL/TLS functionality with proper OpenSSL dependencies and debugging support through key logging.
10262-10305: Well-designed: Modular SSL key logging libraryThe
ssl_key_logginglibrary is properly separated with public visibility, enabling modular usage of SSL/TLS key logging functionality for debugging purposes.
10307-10441: Excellent modular design: Channelz visualization librariesThe zviz library suite provides a well-structured, modular approach to channelz visualization with clear separation of concerns between data handling, layout, and HTML generation.
10701-10704: LGTM: XDS type matcher UPB libraryThe
xds_type_matcher_upblibrary correctly integrates CNCF XDS proto packages for enhanced XDS support.deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc (9)
99-110: LGTM!Proper mutex protection for connectivity state management.
231-250: LGTM!Proper validation to prevent duplicate initial/trailing metadata.
252-291: LGTM!Correct metadata processing with proper differentiation between initial and trailing metadata.
296-313: LGTM!Proper RST_STREAM handling that closes the stream without breaking the connection.
498-515: LGTM!Proper validation of CONTINUATION frame sequencing per RFC 9113.
583-594: LGTM!Correctly sends the HTTP/2 client connection preface on first write.
732-774: LGTM!Idempotent stream closure with proper state management.
776-843: LGTM!Proper transport shutdown with stream cleanup and state management.
919-982: LGTM!Correct implementation of outbound message handling with proper frame construction.
| SHARED_VERSION_CORE = -49 | ||
| SHARED_VERSION_CPP = -1 |
There was a problem hiding this comment.
Shared-library file name and SONAME mismatch will break dynamic loading
SHARED_VERSION_CORE is set to -49, so the output file becomes
libgrpc-49.so.49.0.0, while the -Wl,-soname is libgrpc.so.49 and no
symlink for that name is created. Any binary linked against libgrpc.so.49
will fail at runtime because the loader cannot resolve the SONAME.
Fix by dropping the extra hyphen (or updating SONAME & symlinks
consistently). Minimal patch:
@@
-SHARED_VERSION_CORE = -49
+SHARED_VERSION_CORE =
@@
-$(LDXX) $(LDFLAGS) ... -shared -Wl,-soname,libgrpc.so.49 \
- -o $(LIBDIR)/$(CONFIG)/libgrpc$(SHARED_VERSION_CORE).$(SHARED_EXT_CORE) ...
-ln -sf $(SHARED_PREFIX)grpc$(SHARED_VERSION_CORE).$(SHARED_EXT_CORE) $(LIBDIR)/$(CONFIG)/libgrpc$(SHARED_VERSION_CORE).so.49
-ln -sf $(SHARED_PREFIX)grpc$(SHARED_VERSION_CORE).$(SHARED_EXT_CORE) $(LIBDIR)/$(CONFIG)/libgrpc$(SHARED_VERSION_CORE).so
+$(LDXX) $(LDFLAGS) ... -shared -Wl,-soname,libgrpc.so.49 \
+ -o $(LIBDIR)/$(CONFIG)/libgrpc.$(SHARED_EXT_CORE) ...
+ln -sf $(SHARED_PREFIX)grpc.$(SHARED_EXT_CORE) $(LIBDIR)/$(CONFIG)/libgrpc.so.49
+ln -sf $(SHARED_PREFIX)grpc.$(SHARED_EXT_CORE) $(LIBDIR)/$(CONFIG)/libgrpc.soWithout this correction, every consumer of the new 1.74.0 build will hit
ld.so: cannot open shared object file: No such file or directory.
Also applies to: 864-866
🤖 Prompt for AI Agents
In deps/grpc/Makefile around lines 407 to 408, the SHARED_VERSION_CORE is set
with a leading hyphen causing the output shared library filename to include an
extra hyphen, which mismatches the SONAME and breaks dynamic loading. Fix this
by removing the leading hyphen from SHARED_VERSION_CORE so the output filename
and SONAME align correctly. Also, apply the same fix to lines 864 to 866 to
ensure consistency in shared library versioning and symlink creation.
| class CallCredsFactory final { | ||
| public: | ||
| virtual ~CallCredsFactory() {} | ||
| virtual absl::string_view type() const = delete; | ||
| virtual RefCountedPtr<CallCredsConfig> ParseConfig( | ||
| const Json& config, const JsonArgs& args, | ||
| ValidationErrors* errors) const = delete; | ||
| virtual RefCountedPtr<T> CreateCallCreds( | ||
| RefCountedPtr<CallCredsConfig> config) const = delete; | ||
| }; |
There was a problem hiding this comment.
Fix invalid virtual method declarations
Virtual methods cannot be deleted using = delete. This will cause compilation errors. They should be pure virtual instead.
template <typename T = grpc_call_credentials>
class CallCredsFactory final {
public:
virtual ~CallCredsFactory() {}
- virtual absl::string_view type() const = delete;
+ virtual absl::string_view type() const = 0;
virtual RefCountedPtr<CallCredsConfig> ParseConfig(
const Json& config, const JsonArgs& args,
- ValidationErrors* errors) const = delete;
+ ValidationErrors* errors) const = 0;
virtual RefCountedPtr<T> CreateCallCreds(
- RefCountedPtr<CallCredsConfig> config) const = delete;
+ RefCountedPtr<CallCredsConfig> config) const = 0;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class CallCredsFactory final { | |
| public: | |
| virtual ~CallCredsFactory() {} | |
| virtual absl::string_view type() const = delete; | |
| virtual RefCountedPtr<CallCredsConfig> ParseConfig( | |
| const Json& config, const JsonArgs& args, | |
| ValidationErrors* errors) const = delete; | |
| virtual RefCountedPtr<T> CreateCallCreds( | |
| RefCountedPtr<CallCredsConfig> config) const = delete; | |
| }; | |
| template <typename T = grpc_call_credentials> | |
| class CallCredsFactory final { | |
| public: | |
| virtual ~CallCredsFactory() {} | |
| virtual absl::string_view type() const = 0; | |
| virtual RefCountedPtr<CallCredsConfig> ParseConfig( | |
| const Json& config, const JsonArgs& args, | |
| ValidationErrors* errors) const = 0; | |
| virtual RefCountedPtr<T> CreateCallCreds( | |
| RefCountedPtr<CallCredsConfig> config) const = 0; | |
| }; |
🤖 Prompt for AI Agents
In deps/grpc/src/core/credentials/call/call_creds_registry.h around lines 46 to
55, the virtual methods are incorrectly marked as deleted using '= delete',
which is invalid and causes compilation errors. Change these virtual methods to
be pure virtual by replacing '= delete' with '= 0' to properly declare them as
abstract methods.
| CHECK(!op->set_accept_stream) << "Set_accept_stream not supported on clients"; | ||
| DCHECK(did_stuff) << "Unimplemented transport perform op "; |
There was a problem hiding this comment.
Replace DCHECK with proper error handling for unimplemented operations.
DCHECK is only active in debug builds. In production, unimplemented transport operations will silently pass through without any indication of failure.
Replace with proper error handling:
- DCHECK(did_stuff) << "Unimplemented transport perform op ";
+ if (!did_stuff) {
+ LOG(ERROR) << "Unimplemented transport perform op";
+ // Consider returning an error status to the caller
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CHECK(!op->set_accept_stream) << "Set_accept_stream not supported on clients"; | |
| DCHECK(did_stuff) << "Unimplemented transport perform op "; | |
| CHECK(!op->set_accept_stream) << "Set_accept_stream not supported on clients"; | |
| if (!did_stuff) { | |
| LOG(ERROR) << "Unimplemented transport perform op"; | |
| // Consider returning an error status to the caller | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
around lines 86 to 87, replace the DCHECK that checks for unimplemented
transport operations with proper error handling to ensure failures are detected
in production builds. Instead of DCHECK, implement a runtime error handling
mechanism such as returning an error status or logging an error and aborting the
operation to prevent silent failures in production.
| GRPC_HTTP2_CLIENT_DLOG << "Http2Transport ProcessHttp2SettingsFrame Factory"; | ||
| // TODO(tjagtap) : [PH2][P2] : Implement this. | ||
| // Load into this.settings_ | ||
| // Take necessary actions as per settings that have changed. | ||
| HTTP2_TRANSPORT_DLOG | ||
| GRPC_HTTP2_CLIENT_DLOG | ||
| << "Http2Transport ProcessHttp2SettingsFrame Promise { ack=" << frame.ack | ||
| << ", settings length=" << frame.settings.size() << "}"; | ||
| if (on_receive_settings_ != nullptr) { | ||
| ExecCtx::Run(DEBUG_LOCATION, on_receive_settings_, absl::OkStatus()); | ||
| on_receive_settings_ = nullptr; | ||
| } | ||
| return Http2Status::Ok(); | ||
| } |
There was a problem hiding this comment.
Settings frame content is not processed.
The function acknowledges settings receipt but doesn't actually apply the settings values, which could lead to protocol violations.
At minimum, validate and apply the received settings:
if (on_receive_settings_ != nullptr) {
ExecCtx::Run(DEBUG_LOCATION, on_receive_settings_, absl::OkStatus());
on_receive_settings_ = nullptr;
}
+ // TODO(tjagtap): Apply received settings
+ for (const auto& setting : frame.settings) {
+ // Validate and apply each setting
+ LOG(WARNING) << "Received setting not yet processed: id="
+ << setting.id << " value=" << setting.value;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| GRPC_HTTP2_CLIENT_DLOG << "Http2Transport ProcessHttp2SettingsFrame Factory"; | |
| // TODO(tjagtap) : [PH2][P2] : Implement this. | |
| // Load into this.settings_ | |
| // Take necessary actions as per settings that have changed. | |
| HTTP2_TRANSPORT_DLOG | |
| GRPC_HTTP2_CLIENT_DLOG | |
| << "Http2Transport ProcessHttp2SettingsFrame Promise { ack=" << frame.ack | |
| << ", settings length=" << frame.settings.size() << "}"; | |
| if (on_receive_settings_ != nullptr) { | |
| ExecCtx::Run(DEBUG_LOCATION, on_receive_settings_, absl::OkStatus()); | |
| on_receive_settings_ = nullptr; | |
| } | |
| return Http2Status::Ok(); | |
| } | |
| GRPC_HTTP2_CLIENT_DLOG << "Http2Transport ProcessHttp2SettingsFrame Factory"; | |
| // TODO(tjagtap) : [PH2][P2] : Implement this. | |
| // Load into this.settings_ | |
| // Take necessary actions as per settings that have changed. | |
| GRPC_HTTP2_CLIENT_DLOG | |
| << "Http2Transport ProcessHttp2SettingsFrame Promise { ack=" << frame.ack | |
| << ", settings length=" << frame.settings.size() << "}"; | |
| if (on_receive_settings_ != nullptr) { | |
| ExecCtx::Run(DEBUG_LOCATION, on_receive_settings_, absl::OkStatus()); | |
| on_receive_settings_ = nullptr; | |
| } | |
| // TODO(tjagtap): Apply received settings | |
| for (const auto& setting : frame.settings) { | |
| // Validate and apply each setting | |
| LOG(WARNING) << "Received setting not yet processed: id=" | |
| << setting.id << " value=" << setting.value; | |
| } | |
| return Http2Status::Ok(); | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
around lines 318 to 330, the ProcessHttp2SettingsFrame function currently logs
receipt of the settings frame but does not parse or apply the settings values,
risking protocol violations. Update the function to iterate over the
frame.settings entries, validate each setting according to HTTP/2
specifications, and apply them to this.settings_. After applying, trigger any
necessary state changes or actions based on the updated settings before
returning Http2Status::Ok().
6c2126f to
f14b099
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (25)
deps/grpc/Makefile (2)
89-90: Missing leading dash still present inLDFLAGS_asan-noleaks.
The linker flag is emitted asfsanitize=addressinstead of-fsanitize=address, so the option is still interpreted as a filename. The previous review already pointed this out ‒ please apply the patch (add the leading-) or acknowledge why it is intentionally omitted.
407-408: SONAME / filename mismatch remains – shared objects will not load.
SHARED_VERSION_COREkeeps the leading hyphen (-49), producinglibgrpc-49.so.49.0.0, while the link rule sets-soname,libgrpc.so.49and the created symlinks expectlibgrpc.so.49. The runtime loader will fail to resolve the SONAME.
Same patch as previously suggested: drop the leading hyphen (or update the soname/symlinks consistently).Also applies to: 1864-1866
deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc (1)
344-357: Verify the write size boundary logic change and add tests/docs.The experimental flag introduces a change from
>to>=in the boundary check that needs proper testing and documentation.deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.h (1)
64-67: Improve comparison to use file paths instead of raw pointers.The current pointer-based comparison provides unstable ordering. Consider comparing based on the file path for stable, meaningful ordering.
deps/grpc/src/core/ext/transport/chttp2/client/chttp2_connector.cc (1)
209-211: Fix error handling to prevent null endpoint usage.The code logs an error but continues execution with a null endpoint, risking a crash.
deps/grpc/src/core/client_channel/retry_throttle.h (2)
41-44: Make constructor private to enforce factory pattern.The comment says "Do not instantiate directly -- use Create() instead" but the constructor is public. Make it private and add
Createas a friend function if needed.static UniqueTypeName Type(); + private: + friend RefCountedPtr<RetryThrottler> Create( + uintptr_t max_milli_tokens, uintptr_t milli_token_ratio, + RefCountedPtr<RetryThrottler> previous); + // Do not instantiate directly -- use Create() instead. RetryThrottler(uintptr_t max_milli_tokens, uintptr_t milli_token_ratio, uintptr_t milli_tokens); + + public: ~RetryThrottler() override;
60-62: Document the replacement mechanism methods.The
SetReplacementandGetReplacementThrottleDataIfNeededmethods lack documentation. Their purpose and thread-safety guarantees should be clearly documented.+ // Sets a replacement throttler for this instance. Thread-safe. void SetReplacement(RefCountedPtr<RetryThrottler> replacement); + // Updates throttle_data to point to the current replacement if one exists. + // Thread-safe. Used to ensure the most recent throttler is always used. void GetReplacementThrottleDataIfNeeded(RetryThrottler** throttle_data);deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc (2)
659-661: Document and track Python experimental Event Engine flag.The addition of
EventEngineExperimentDisabledForPython()check suggests this feature might have compatibility issues with Python bindings. Ensure this is documented and tracked.Consider:
- Adding a comment in the code explaining why Python requires special handling
- Creating a tracking issue for Python Event Engine compatibility
- Documenting this limitation in the Event Engine feature documentation
242-244: Consider softer error handling in FdHandler::Handle.The use of
CHECKat line 242 will crash the process ifHandleExternalConnectionfails. Consider using proper error logging and graceful handling instead.- CHECK(GRPC_LOG_IF_ERROR("listener_handle_external_connection", - listener_supports_fd_->HandleExternalConnection( - listener_fd, fd, &pending_data))); + auto status = listener_supports_fd_->HandleExternalConnection( + listener_fd, fd, &pending_data); + if (!status.ok()) { + LOG(ERROR) << "Failed to handle external connection: " << status; + // Consider closing the fd or other cleanup + close(fd); + }deps/grpc/src/core/credentials/call/call_creds_registry.h (1)
49-54: Fix invalid virtual method declarations.Virtual methods cannot be deleted using
= delete. This will cause compilation errors. They should be pure virtual instead.template <typename T = grpc_call_credentials> class CallCredsFactory final { public: virtual ~CallCredsFactory() {} - virtual absl::string_view type() const = delete; + virtual absl::string_view type() const = 0; virtual RefCountedPtr<CallCredsConfig> ParseConfig( const Json& config, const JsonArgs& args, - ValidationErrors* errors) const = delete; + ValidationErrors* errors) const = 0; virtual RefCountedPtr<T> CreateCallCreds( - RefCountedPtr<CallCredsConfig> config) const = delete; + RefCountedPtr<CallCredsConfig> config) const = 0; };deps/grpc/src/core/ext/transport/chttp2/transport/header_assembler.h (3)
99-101: Consider using runtime validation instead of DCHECK for stream ID.DCHECK only validates in debug builds. Since this validates external input (HTTP/2 frames from peers), consider using runtime validation that works in all builds.
- DCHECK_GT(frame.stream_id, 0u) - << "RFC9113 : HEADERS frames MUST be associated with a stream."; + if (frame.stream_id == 0) { + Cleanup(); + LOG(ERROR) << "RFC9113 : HEADERS frames MUST be associated with a stream."; + return Http2Status::Http2ConnectionError( + Http2ErrorCode::kProtocolError, + "HEADERS frames MUST be associated with a stream."); + }
134-147: Add validation checks for CONTINUATION frames.The
AppendContinuationFramemethod lacks important validations that are present inAppendHeaderFrame:
- Verify that a HEADERS frame was received before CONTINUATION
- Validate stream ID matches the initial HEADERS frame
- Check for buffer overflow on 32-bit platforms
Http2Status AppendContinuationFrame(Http2ContinuationFrame&& frame) { + // Validate header is in progress + if (!header_in_progress_) { + Cleanup(); + LOG(ERROR) << kAssemblerContiguousSequenceError; + return Http2Status::Http2ConnectionError( + Http2ErrorCode::kProtocolError, + std::string(kAssemblerContiguousSequenceError)); + } + + // Validate stream ID matches + if (frame.stream_id != stream_id_) { + Cleanup(); + LOG(ERROR) << kAssemblerMismatchedStreamId; + return Http2Status::Http2ConnectionError( + Http2ErrorCode::kProtocolError, + std::string(kAssemblerMismatchedStreamId)); + } + + // Check for overflow on 32-bit platforms + const size_t current_len = frame.payload.Length(); + if constexpr (sizeof(size_t) == 4) { + if (GPR_UNLIKELY(buffer_.Length() >= UINT32_MAX - current_len)) { + Cleanup(); + LOG(ERROR) + << "Stream Error: SliceBuffer overflow for 32 bit platforms."; + return Http2Status::Http2StreamError( + Http2ErrorCode::kInternalError, + "Stream Error: SliceBuffer overflow for 32 bit platforms."); + } + } + // Manage payload - const size_t current_len = frame.payload.Length(); frame.payload.MoveFirstNBytesIntoSliceBuffer(current_len, buffer_);
165-169: Implement proper metadata size limits.The TODO indicates that metadata size limits need to be managed. Using
std::numeric_limits<uint32_t>::max()for both soft and hard limits effectively disables size enforcement, which could lead to memory exhaustion attacks.Would you like me to help implement configurable metadata size limits based on channel arguments or system constraints?
deps/grpc/src/core/channelz/channelz.h (1)
130-147: Add default case or unreachable annotation to EntityTypeToKind.The switch statement doesn't have a default case or return statement after the switch, which could cause undefined behavior if the enum is extended.
static absl::string_view EntityTypeToKind(EntityType type) { switch (type) { case EntityType::kTopLevelChannel: return "channel"; case EntityType::kInternalChannel: return "internal_channel"; case EntityType::kSubchannel: return "subchannel"; case EntityType::kServer: return "server"; case EntityType::kListenSocket: return "listen_socket"; case EntityType::kSocket: return "socket"; case EntityType::kCall: return "call"; } + GPR_UNREACHABLE_CODE(return "unknown"); }deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h (1)
118-127: Ensure proper synchronization for scheduling state cleanupThe destructor reads
scheduling_state_with acquire ordering but the state transitions might need stronger ordering to ensure the Waker pointer is visible.#!/bin/bash # Check how scheduling_state_ is updated in the implementation rg -A 5 "scheduling_state_.*store" deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.ccdeps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h (2)
507-524: Address incomplete error handling and consider lifetime management.The TODOs indicate incomplete implementation:
- GOAWAY triggering on ping timeout needs implementation
- Error code choice needs review per RFC9113
The raw pointer storage issue mentioned in previous reviews still applies - consider using RefCountedPtr for transport lifetime safety.
536-581: Address lifetime management and incomplete error handling.Similar issues as
PingSystemInterfaceImpl:
- Raw pointer storage creates dangling pointer risk (lines 580)
- TODOs indicate incomplete GOAWAY triggering (lines 553, 556-560)
- Error code selection needs RFC9113 review
Consider using
RefCountedPtr<Http2ClientTransport>and implementing the suggested ref-counted structure mentioned in TODOs.deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc (2)
274-275: Replace LOG(FATAL) with safer alternative.Using
LOG(FATAL)will crash the process. Consider usingLOG(DFATAL)for debug builds only or implement graceful error handling.- LOG(FATAL) << "Unreachable state: " << state; + LOG(DFATAL) << "Unreachable state: " << state; + return; // Defensive fallback
304-305: Replace LOG(FATAL) with safer alternative.Same issue as line 274 - avoid process crashes in production code.
- LOG(FATAL) << "Unreachable state: " << state; + LOG(DFATAL) << "Unreachable state: " << state; + return Pending{}; // Defensive fallbackdeps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc (6)
86-87: Replace DCHECK with proper error handling for unimplemented operations.
DCHECKis only active in debug builds. In production, unimplemented transport operations will silently pass through without any indication of failure.
217-229: Incomplete stream validation may cause protocol violations.The header frame processing returns
Ok()when stream lookup fails, but the comment indicates this should be a PROTOCOL_ERROR per RFC 9113.
318-330: Settings frame content is not processed.The function acknowledges settings receipt but doesn't actually apply the settings values, which could lead to protocol violations.
371-379: GOAWAY frame processing is not implemented.The GOAWAY frame handler has a TODO but returns
Ok(). This could cause the client to continue using a connection that the server is shutting down.
714-718: Handle failure from Apply when setting initial window size.The call to
settings_.mutable_local().Apply(...)usesDCHECK, which is stripped out in release builds. Runtime checks are needed to catch and handle failures in production.
869-870: Add stream ID validation per RFC 9113.Stream identifiers must be validated according to RFC 9113 requirements (e.g., client-initiated streams must use odd stream IDs).
🧹 Nitpick comments (12)
deps/grpc/src/core/credentials/transport/google_default/google_default_credentials.cc (1)
435-480: Extended API properly supports dual credentials while maintaining compatibility.The implementation correctly handles both single TLS credentials and dual TLS/ALTS credentials cases. Consider adding documentation in the header file to clarify when to use each parameter.
deps/grpc/src/core/channelz/channel_trace.h (1)
152-156: Consider using a more descriptive constant name for sentinel IDThe name
kSentinelIdis used but its value is defined later (line 305). Consider moving the constant definition closer to whereEntryRefis defined or add a comment explaining what the sentinel represents.deps/grpc/src/core/channelz/property_list.h (1)
98-104: Ensure T is copyable/movable for make_sharedThe
Wrapperspecialization for final classes derived fromOtherPropertyValuecreates ashared_ptrby value. This requiresTto be copyable or movable, which should be documented or enforced with a static_assert.template <typename T> struct Wrapper<T, std::enable_if_t<std::is_base_of_v<OtherPropertyValue, T> && std::is_final_v<T>>> { + static_assert(std::is_copy_constructible_v<T> || std::is_move_constructible_v<T>, + "Type T must be copy or move constructible"); static std::optional<PropertyValue> Wrap(T value) { return PropertyValue(std::make_shared<T>(std::move(value))); } };deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h (1)
141-142: Add assertion message for better debuggingThe destructor uses
CHECK(dropped_)without a message. Adding a descriptive message would help with debugging.- ~Reader() { CHECK(dropped_); } + ~Reader() { CHECK(dropped_) << "Reader destroyed without calling Drop()"; }deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h (1)
126-138: LGTM! Consider the optimization TODO.The explicit token count passing (line 130) is correct. The TODO suggests a potential optimization by creating the sender in the constructor - this could reduce allocation overhead in the hot path.
deps/grpc/src/core/BUILD (7)
92-107:memory_usagelibrary seems header-only but carries heavy deps
memory_usageexports onlyutil/memory_usage.h, yet depends onabsl/status,absl/time, etc. Unless the header actually requires those types, this inflates the dependency graph for every target that transitively includes it.Double-check whether the header can be made self-contained or move the implementation into a
.ccfile to limit the dependency surface.
162-163: Order of declaration vs. usage
event_engine_commonnow depends oncontext_list_entry, which is declared ~5 000 lines later. Bazel allows this, but it hurts readability and complicates manual grep/debug sessions.Consider moving the
context_list_entrytarget closer to its first consumer.
2459-2488: Duplicate POSIX interface dep—compile-time bloatSeveral poller variants (
posix_poll,epoll1) now both depend onposix_event_engine_posix_interface. Ensure that the interface code is really header-only or addalwayslink = 0to avoid duplicate object code.
2560-2576:posix_write_event_sinkmissing visibilityThe library is private by default but is required by
posix_event_engine_traced_buffer_list. Add an explicitvisibilitystanza or merge the targets.
3143-3152:endpoint_channel_arg_wrapperleaks core headers to public APIsThis helper exposes
grpc_public_hdrsdirectly; downstream consumers that only need channel args will now inherit a lot of transitive public headers. Consider hiding the impl behind an internal target and exposing a thinner wrapper.
8181-8208:transport_commonis effectively emptyA new library is declared with sources but no deps. If this is intentional (e.g., a forthcoming implementation) add a
TODOcomment; otherwise delete until needed to keep the BUILD clean.
10193-10215:channelz_property_listpulls in heavy upb reflection
channelz_property_listadds@upb/textand friends, which adds ~0.5 MB to most binaries. If reflection/text formatting is only required for debug builds, consider moving those deps under aselect()on a//:enable_channelz_zvizfeature.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (154)
deps/grpc/.bazelci/presubmit.yml(1 hunks)deps/grpc/Makefile(22 hunks)deps/grpc/grpc.gyp(11 hunks)deps/grpc/include/grpc/create_channel_from_endpoint.h(1 hunks)deps/grpc/include/grpc/credentials.h(1 hunks)deps/grpc/include/grpc/event_engine/event_engine.h(7 hunks)deps/grpc/include/grpc/grpc_posix.h(1 hunks)deps/grpc/include/grpc/impl/channel_arg_names.h(1 hunks)deps/grpc/include/grpc/module.modulemap(1 hunks)deps/grpc/include/grpc/support/json.h(1 hunks)deps/grpc/include/grpcpp/channel.h(2 hunks)deps/grpc/include/grpcpp/create_channel_posix.h(2 hunks)deps/grpc/include/grpcpp/impl/generic_serialize.h(3 hunks)deps/grpc/include/grpcpp/security/credentials.h(2 hunks)deps/grpc/include/grpcpp/support/callback_common.h(2 hunks)deps/grpc/include/grpcpp/support/client_callback.h(3 hunks)deps/grpc/include/grpcpp/version_info.h(1 hunks)deps/grpc/src/compiler/php_generator.cc(1 hunks)deps/grpc/src/core/BUILD(96 hunks)deps/grpc/src/core/call/filter_fusion.h(2 hunks)deps/grpc/src/core/call/interception_chain.h(4 hunks)deps/grpc/src/core/channelz/channel_trace.cc(1 hunks)deps/grpc/src/core/channelz/channel_trace.h(3 hunks)deps/grpc/src/core/channelz/channelz.cc(20 hunks)deps/grpc/src/core/channelz/channelz.h(20 hunks)deps/grpc/src/core/channelz/channelz_registry.cc(1 hunks)deps/grpc/src/core/channelz/channelz_registry.h(3 hunks)deps/grpc/src/core/channelz/property_list.cc(1 hunks)deps/grpc/src/core/channelz/property_list.h(1 hunks)deps/grpc/src/core/channelz/ztrace_collector.h(3 hunks)deps/grpc/src/core/channelz/zviz/data.cc(1 hunks)deps/grpc/src/core/channelz/zviz/data.h(1 hunks)deps/grpc/src/core/channelz/zviz/entity.cc(1 hunks)deps/grpc/src/core/channelz/zviz/entity.h(1 hunks)deps/grpc/src/core/channelz/zviz/environment.cc(1 hunks)deps/grpc/src/core/channelz/zviz/environment.h(1 hunks)deps/grpc/src/core/channelz/zviz/html.cc(1 hunks)deps/grpc/src/core/channelz/zviz/html.h(1 hunks)deps/grpc/src/core/channelz/zviz/layout.cc(1 hunks)deps/grpc/src/core/channelz/zviz/layout.h(1 hunks)deps/grpc/src/core/channelz/zviz/layout_html.cc(1 hunks)deps/grpc/src/core/channelz/zviz/layout_html.h(1 hunks)deps/grpc/src/core/channelz/zviz/strings.cc(1 hunks)deps/grpc/src/core/channelz/zviz/strings.h(1 hunks)deps/grpc/src/core/channelz/zviz/trace.cc(1 hunks)deps/grpc/src/core/channelz/zviz/trace.h(1 hunks)deps/grpc/src/core/client_channel/client_channel.cc(5 hunks)deps/grpc/src/core/client_channel/client_channel_filter.cc(4 hunks)deps/grpc/src/core/client_channel/config_selector.h(2 hunks)deps/grpc/src/core/client_channel/dynamic_filters.cc(2 hunks)deps/grpc/src/core/client_channel/dynamic_filters.h(1 hunks)deps/grpc/src/core/client_channel/global_subchannel_pool.cc(1 hunks)deps/grpc/src/core/client_channel/retry_filter.cc(1 hunks)deps/grpc/src/core/client_channel/retry_filter.h(4 hunks)deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.cc(3 hunks)deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.h(1 hunks)deps/grpc/src/core/client_channel/retry_interceptor.cc(5 hunks)deps/grpc/src/core/client_channel/retry_interceptor.h(3 hunks)deps/grpc/src/core/client_channel/retry_throttle.cc(3 hunks)deps/grpc/src/core/client_channel/retry_throttle.h(1 hunks)deps/grpc/src/core/client_channel/subchannel.cc(6 hunks)deps/grpc/src/core/client_channel/subchannel.h(2 hunks)deps/grpc/src/core/config/config_vars.cc(1 hunks)deps/grpc/src/core/config/config_vars.yaml(1 hunks)deps/grpc/src/core/config/core_configuration.cc(1 hunks)deps/grpc/src/core/config/core_configuration.h(5 hunks)deps/grpc/src/core/credentials/call/call_creds_registry.h(1 hunks)deps/grpc/src/core/credentials/call/call_creds_registry_init.cc(1 hunks)deps/grpc/src/core/credentials/call/gcp_service_account_identity/gcp_service_account_identity_credentials.cc(2 hunks)deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.cc(1 hunks)deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.h(1 hunks)deps/grpc/src/core/credentials/call/jwt_util.cc(1 hunks)deps/grpc/src/core/credentials/call/jwt_util.h(1 hunks)deps/grpc/src/core/credentials/transport/channel_creds_registry_init.cc(1 hunks)deps/grpc/src/core/credentials/transport/google_default/google_default_credentials.cc(4 hunks)deps/grpc/src/core/credentials/transport/ssl/ssl_credentials.cc(0 hunks)deps/grpc/src/core/credentials/transport/tls/load_system_roots_supported.cc(1 hunks)deps/grpc/src/core/credentials/transport/xds/xds_credentials.cc(0 hunks)deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc(1 hunks)deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h(1 hunks)deps/grpc/src/core/ext/filters/http/client_authority_filter.cc(1 hunks)deps/grpc/src/core/ext/filters/http/message_compress/compression_filter.h(4 hunks)deps/grpc/src/core/ext/filters/http/server/http_server_filter.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/client_transport.cc(6 hunks)deps/grpc/src/core/ext/transport/chaotic_good/client_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/config.h(4 hunks)deps/grpc/src/core/ext/transport/chaotic_good/control_endpoint.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc(6 hunks)deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h(5 hunks)deps/grpc/src/core/ext/transport/chaotic_good/frame.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/frame_transport.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/message_chunker.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/scheduler.cc(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/scheduler.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc(11 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h(5 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server_transport.cc(5 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_header.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.cc(7 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_ztrace_collector.h(3 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/chaotic_good_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/client_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/server/chaotic_good_server.cc(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.cc(4 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/client/chttp2_connector.cc(7 hunks)deps/grpc/src/core/ext/transport/chttp2/server/chttp2_server.cc(4 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.cc(17 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.h(0 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/flow_control.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame_data.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame_settings.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame_window_update.cc(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/header_assembler.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc(5 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.h(4 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc(15 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h(11 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.cc(10 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_status.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_transport.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_ztrace_collector.h(0 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/internal.h(6 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.cc(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.h(4 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/message_assembler.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/parsing.cc(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_callbacks.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.cc(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.h(0 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/stream_data_queue.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/stream_lists.cc(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc(10 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.c(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.c(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/property_list.upb.h(1 hunks)
💤 Files with no reviewable changes (5)
- deps/grpc/src/core/credentials/transport/xds/xds_credentials.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.h
- deps/grpc/src/core/credentials/transport/ssl/ssl_credentials.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_ztrace_collector.h
✅ Files skipped from review due to trivial changes (13)
- deps/grpc/src/core/ext/transport/chttp2/transport/stream_data_queue.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.cc
- deps/grpc/include/grpcpp/version_info.h
- deps/grpc/src/core/channelz/ztrace_collector.h
- deps/grpc/src/core/channelz/zviz/strings.h
- deps/grpc/include/grpc/create_channel_from_endpoint.h
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/chaotic_good_transport.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_transport.h
- deps/grpc/src/core/credentials/call/jwt_util.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/property_list.upb.h
- deps/grpc/.bazelci/presubmit.yml
🚧 Files skipped from review as they are similar to previous changes (113)
- deps/grpc/src/core/credentials/transport/tls/load_system_roots_supported.cc
- deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.cc
- deps/grpc/src/core/client_channel/global_subchannel_pool.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/frame.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/parsing.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.cc
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/server/chaotic_good_server.cc
- deps/grpc/src/core/channelz/channelz_registry.cc
- deps/grpc/src/core/client_channel/config_selector.h
- deps/grpc/src/core/ext/transport/chttp2/transport/stream_lists.cc
- deps/grpc/src/core/config/config_vars.cc
- deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h
- deps/grpc/include/grpc/grpc_posix.h
- deps/grpc/src/core/ext/transport/chttp2/transport/frame_settings.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.h
- deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.h
- deps/grpc/src/core/config/config_vars.yaml
- deps/grpc/src/core/channelz/zviz/data.h
- deps/grpc/src/core/ext/transport/chaotic_good/scheduler.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc
- deps/grpc/src/core/channelz/zviz/entity.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_callbacks.h
- deps/grpc/include/grpc/credentials.h
- deps/grpc/src/core/channelz/zviz/trace.h
- deps/grpc/include/grpcpp/impl/generic_serialize.h
- deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.h
- deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.h
- deps/grpc/src/core/config/core_configuration.h
- deps/grpc/src/core/client_channel/dynamic_filters.cc
- deps/grpc/src/compiler/php_generator.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/message_assembler.h
- deps/grpc/src/core/ext/transport/chaotic_good/server_transport.h
- deps/grpc/src/core/channelz/zviz/environment.h
- deps/grpc/src/core/ext/transport/chaotic_good/scheduler.cc
- deps/grpc/src/core/call/interception_chain.h
- deps/grpc/include/grpc/impl/channel_arg_names.h
- deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.h
- deps/grpc/src/core/ext/transport/chaotic_good/control_endpoint.h
- deps/grpc/src/core/channelz/zviz/entity.cc
- deps/grpc/src/core/ext/transport/chaotic_good/frame.h
- deps/grpc/src/core/ext/transport/chaotic_good/server_transport.cc
- deps/grpc/include/grpc/support/json.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.c
- deps/grpc/src/core/credentials/call/jwt_util.cc
- deps/grpc/src/core/channelz/zviz/layout_html.cc
- deps/grpc/include/grpcpp/security/credentials.h
- deps/grpc/include/grpcpp/create_channel_posix.h
- deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.cc
- deps/grpc/src/core/ext/transport/chttp2/server/chttp2_server.cc
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb.h
- deps/grpc/src/core/channelz/zviz/layout_html.h
- deps/grpc/src/core/channelz/zviz/data.cc
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.h
- deps/grpc/src/core/client_channel/client_channel_filter.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings.h
- deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.cc
- deps/grpc/src/core/credentials/call/call_creds_registry_init.cc
- deps/grpc/grpc.gyp
- deps/grpc/src/core/ext/filters/http/message_compress/compression_filter.h
- deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_header.h
- deps/grpc/src/core/channelz/zviz/html.h
- deps/grpc/src/core/client_channel/client_channel.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/frame_data.cc
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.c
- deps/grpc/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc
- deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h
- deps/grpc/include/grpc/module.modulemap
- deps/grpc/src/core/client_channel/retry_interceptor.h
- deps/grpc/src/core/client_channel/retry_filter.h
- deps/grpc/src/core/ext/filters/http/client_authority_filter.cc
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/client_transport.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_status.h
- deps/grpc/src/core/credentials/transport/channel_creds_registry_init.cc
- deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser.cc
- deps/grpc/src/core/ext/transport/chaotic_good/config.h
- deps/grpc/src/core/ext/transport/chaotic_good/frame_transport.h
- deps/grpc/src/core/channelz/zviz/layout.h
- deps/grpc/src/core/config/core_configuration.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/frame.h
- deps/grpc/src/core/ext/transport/chaotic_good/client_transport.cc
- deps/grpc/src/core/channelz/zviz/strings.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/frame_window_update.cc
- deps/grpc/src/core/client_channel/subchannel.h
- deps/grpc/include/grpcpp/channel.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.cc
- deps/grpc/src/core/ext/transport/chaotic_good/client_transport.h
- deps/grpc/src/core/channelz/channelz.cc
- deps/grpc/src/core/client_channel/subchannel.cc
- deps/grpc/src/core/client_channel/retry_interceptor.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.h
- deps/grpc/src/core/ext/transport/chaotic_good/message_chunker.h
- deps/grpc/src/core/channelz/zviz/environment.cc
- deps/grpc/src/core/client_channel/dynamic_filters.h
- deps/grpc/src/core/ext/filters/http/server/http_server_filter.h
- deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc
- deps/grpc/src/core/credentials/call/gcp_service_account_identity/gcp_service_account_identity_credentials.cc
- deps/grpc/include/grpcpp/support/callback_common.h
- deps/grpc/src/core/channelz/zviz/layout.cc
- deps/grpc/src/core/channelz/zviz/trace.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/flow_control.h
- deps/grpc/include/grpcpp/support/client_callback.h
- deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.cc
- deps/grpc/src/core/call/filter_fusion.h
- deps/grpc/src/core/channelz/channelz_registry.h
- deps/grpc/src/core/channelz/zviz/html.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/internal.h
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.cc
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.h
- deps/grpc/src/core/client_channel/retry_throttle.cc
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb.h
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: in nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional javascript-facing argume...
Learnt from: santigimeno
PR: nodesource/nsolid#339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
Applied to files:
deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc
📚 Learning: the `span` class in `doc/api/diagnostics_channel.md` is unrelated to the one in `lib/internal/otel/t...
Learnt from: santigimeno
PR: nodesource/nsolid#289
File: lib/internal/otel/trace.js:40-40
Timestamp: 2025-04-16T06:58:59.313Z
Learning: The `Span` class in `doc/api/diagnostics_channel.md` is unrelated to the one in `lib/internal/otel/trace.js`. They share the same name but serve completely different purposes - one is for documentation examples, while the other is part of the OpenTelemetry implementation.
Applied to files:
deps/grpc/src/core/channelz/channel_trace.h
📚 Learning: in the nsolid codebase, test files in test/addons/ consistently use assert() for input validation an...
Learnt from: santigimeno
PR: nodesource/nsolid#327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
Applied to files:
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-tarball-linux
- GitHub Check: test-macOS
- GitHub Check: lint-js-and-md
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
🔇 Additional comments (67)
deps/grpc/Makefile (1)
370-371: Cross-check core version macro in public headers
I verified thatGRPC_CPP_VERSION_STRINGindeps/grpc/include/grpcpp/version_info.hhas been bumped to1.74.0, but I could not find any public header defining the core version49.0.0. Please:
- Identify which header (for example a generated
grpc/version.hor similar) is supposed to expose the core version.- Update that header so it reflects
CORE_VERSION = 49.0.0, ensuring downstream#ifguards remain correct.deps/grpc/src/core/client_channel/retry_filter.cc (2)
95-112: LGTM! Clean blackboard-based retry throttling implementation.The refactoring to use blackboard-based state management is well-structured. The method properly handles null checks and reuses existing throttlers when available.
114-122: Constructor properly initializes from channel args and blackboard.The initialization correctly retrieves all necessary objects and configuration.
deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc (2)
376-381: TCP call tracer integration looks good.The implementation properly handles both legacy and new TCP trace collection methods with appropriate null checks and conditional logic.
Also applies to: 737-742
142-142: Enhanced telemetry with transport-specific HTTP/2 stats.Good addition of granular statistics collection for various HTTP/2 events.
Also applies to: 263-264, 288-288, 557-557, 564-564
deps/grpc/src/core/ext/transport/chttp2/client/chttp2_connector.cc (2)
118-141: Well-structured endpoint extraction with proper fallback.The code correctly handles both pre-existing endpoints from channel args and fallback to TCP connect handshaker setup.
172-237: Transport selection implementation is clear, but note the temporary macro.The branching between legacy and promise-based transports is well-implemented. The extensive comments about
GRPC_EXPERIMENTAL_TEMPORARILY_DISABLE_PH2being temporary (8-15 months) are helpful for future maintenance.deps/grpc/src/core/credentials/transport/google_default/google_default_credentials.cc (1)
346-399: Well-designed credentials wrapper for TLS/ALTS selection.The implementation correctly selects between TLS and ALTS credentials based on the transport security type from the auth context. Proper lifecycle management for both credential types.
deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc (1)
83-104: Good abstraction for consistent logging.The
LogInitFailureandLogInformationalhelper functions provide excellent abstraction for logging to both standard logs and channelz when available. This promotes consistency across the codebase.deps/grpc/src/core/channelz/channel_trace.cc (1)
49-91: Well-structured refactoring with improved hierarchical support.The refactoring from linked-list to vector-based storage with hierarchical parent-child relationships is well-designed. The memory management with automatic eviction and proper tracking is robust.
deps/grpc/include/grpc/event_engine/event_engine.h (8)
25-30: LGTM! Necessary includes for new functionality.The added includes support the new telemetry abstractions with
std::shared_ptr,std::optional, andstd::utilityfor move semantics.
248-250: Important documentation for callback lifecycle management.The comment correctly emphasizes that callers must ensure endpoint validity when invoking WriteEventCallback, preventing use-after-free issues.
256-263: Well-designed abstraction for metric key management.The
MetricsSetinterface provides a clean abstraction for checking which metrics are requested, enabling efficient metric collection.
271-283: Improved design with shared ownership of MetricsSet.Using
std::shared_ptr<MetricsSet>instead of a span enables better lifecycle management and allows multiple components to share the same metrics configuration.
310-331: Correct move semantics implementation.The move constructor and assignment operator properly transfer ownership of resources, correctly nullifying the source's
google_specific_pointer to prevent double-deletion. The self-assignment check in the move assignment operator is appropriate.
354-356: Good API design for deprecation path.The
TakeDeprecatedAndDiscouragedGoogleSpecificPointermethod properly transfers ownership usingstd::exchange, ensuring the pointer is nullified after transfer. This is safer than the existing getter and supports migration away from this deprecated API.
378-400: Well-structured telemetry abstraction.The
TelemetryInfointerface provides a comprehensive API for metric discovery and set creation. The use ofstd::optionalfor lookups andstd::shared_ptrfor MetricsSet instances follows modern C++ best practices.
429-429: Completes the telemetry integration in Endpoint interface.Adding
GetTelemetryInfo()to the Endpoint interface enables consistent telemetry access across all endpoint implementations.deps/grpc/src/core/ext/transport/chaotic_good/tcp_ztrace_collector.h (5)
80-80: LGTM! Adds endpoint identification to trace data.The addition of
endpoint_idfield properly tracks which endpoint generated the metrics, improving observability.Also applies to: 90-90
94-109: Well-designed channel scheduling trace structure.The
TraceScheduledChannelstruct captures comprehensive scheduling metrics including bandwidth (bytes_per_second) and token bucket state (allowed_bytes), properly supporting the new scheduler infrastructure.
111-133: Comprehensive scheduling trace with proper memory accounting.The
TraceWriteSchedulestruct effectively captures scheduling decisions across multiple channels. TheMemoryUsage()calculation correctly accounts for the dynamic vector allocation, and the JSON rendering provides detailed observability data.
135-146: Updated trace structure for large frame scheduling.The redefined
WriteLargeFrameHeaderTraceproperly captures frame routing decisions withchosen_endpoint, supporting the new multi-endpoint scheduling capabilities.
217-218: Type alias correctly updated for new trace types.The
TcpZTraceCollectortype alias properly includes the newTraceWriteScheduleand reflects the removal of obsolete trace types mentioned in the AI summary.deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.cc (14)
19-19: LGTM: New includes support telemetry and channelz enhancements.These includes align with the transport modernization described in the PR summary, adding support for credentials handling, time operations, channelz property lists, HTTP/2 stats collection, and TCP tracing.
Also applies to: 59-59, 62-62, 72-72, 113-114
161-162: Function signature enhancement for telemetry support.The addition of
std::vector<TcpCallTracerWithOffset> tcp_call_tracersparameter enables detailed TCP event metric recording during writes, which is consistent with the telemetry modernization goals.
242-247: Well-organized type aliases for telemetry components.The type aliases improve code readability and maintainability by shortening the fully qualified EventEngine::Endpoint types used throughout the telemetry implementation.
368-369: Explicit resource cleanup in destructor.Adding explicit
event_engine.reset()ensures proper cleanup ordering and resource management during transport destruction.
372-372: Simplified context list lifecycle management.The direct deletion approach is cleaner and aligns with the overall simplification of transport lifecycle management mentioned in the AI summary.
518-518: Proper flow control window size synchronization.Setting the flow control target to match the initial window size ensures consistency between HTTP/2 settings and flow control mechanisms.
564-642: Excellent modernization of channelz data reporting.The transition from manual JSON construction to structured
grpc_core::channelz::PropertyListusage significantly improves:
- Code maintainability and readability
- Type safety
- Data structure consistency
- Rich property reporting with detailed transport state
The comprehensive property set includes important transport metrics like flow control stats, ping policies, goaway states, and write states with human-readable string representations.
785-815: Well-integrated statistics collection and channelz setup.The initialization properly:
- Creates HTTP2 stats collector from auth context
- Associates the collector with the HPACK parser
- Sets up channelz socket with proper endpoint integration
- Handles conditional channelz enablement correctly
The code follows good practices by checking for null pointers and properly configuring the event engine endpoint extension.
827-827: Proper channelz data source cleanup.Resetting the channelz data source in the
Orphanmethod ensures proper resource cleanup and prevents potential use-after-free issues during transport destruction.
1152-1152: Enhanced write action with telemetry support.Passing
std::move(r.tcp_call_tracers)to the write_action enables detailed TCP event metric recording and aligns with the modernized telemetry infrastructure.
1171-1241: Comprehensive telemetry integration in write operations.The refactored
write_actionmethod effectively integrates telemetry capabilities:Strengths:
- Proper resource management with context list transfer
- Comprehensive telemetry setup with metrics collection
- Proper null pointer checks and conditional telemetry setup
- Clean separation of concerns between transport and telemetry
Implementation details:
- Context list ownership transfer prevents memory leaks
- TCP call tracer integration enables detailed performance monitoring
- Event sink setup captures important write events (SendMsg, Scheduled, Sent, Acked)
- Metric name resolution and TCP event recording provide rich diagnostics
The code correctly handles the case where telemetry info might be unavailable and gracefully degrades functionality.
1451-1451: Proper statistics collection through dedicated collector.Replacing global HTTP/2 send message size statistics with calls to the transport's
http2_statscollector provides better encapsulation and more accurate per-transport metrics.Also applies to: 1611-1611
796-796: Minor formatting improvement.The blank line addition improves code readability by providing visual separation in the constructor logic.
1451-1451: Streamlined RST_STREAM logic.Removing the feature flag check for RST_STREAM sending simplifies the cancel stream logic and suggests the feature has been stabilized and is now always enabled.
deps/grpc/src/core/channelz/channel_trace.h (2)
457-461: Well-designed macro avoiding common pitfallsThe
GRPC_CHANNELZ_LOGmacro implementation cleverly uses a for-loop to ensure statement-level semantics and avoid the dangling-else problem. The extensive comment documentation is excellent.
160-161: Validate max_memory boundary to prevent overflowThe constructor caps
max_memoryatsizeof(Entry) * 32768but doesn't validate if the input is negative when cast to size_t, which could lead to unexpected behavior.explicit ChannelTrace(size_t max_memory) - : max_memory_(std::min(max_memory, sizeof(Entry) * 32768)) {} + : max_memory_(max_memory > sizeof(Entry) * 32768 ? sizeof(Entry) * 32768 : max_memory) {}Likely an incorrect or invalid review comment.
deps/grpc/src/core/channelz/property_list.h (1)
1-1: LGTM! Copyright year is currentThe copyright year 2025 is correctly set for this new file.
deps/grpc/src/core/channelz/channelz.h (1)
329-343: Well-designed template for type-safe data additionThe templated
AddDatamethod with SFINAE to detectTakeJsonObject()method is elegant and type-safe. The nestedDataImplclass properly implements the virtual interface.deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h (1)
414-445: LGTM! Well-designed thread-safe queueThe
SecureFrameQueueclass provides a clean, thread-safe interface for buffering frames with proper mutex protection and waker support.deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h (8)
88-97: LGTM! Good defensive programming practices.The explicit deletion of copy and move constructors/operators prevents accidental copying of this resource-managing transport class. The additional
on_receive_settingsparameter enhances the constructor interface for settings handling.
114-117: LGTM! Essential connectivity state management.These methods provide necessary connectivity state watching functionality for client transport lifecycle management.
215-216: LGTM! Clean write cycle triggering mechanism.Enqueuing an empty frame is an elegant way to force transport write cycles when needed.
230-284: LGTM! Proper HTTP/2 stream state management.The stream state transition methods correctly implement the HTTP/2 stream state machine:
- kIdle → kOpen → kHalfClosedLocal/kHalfClosedRemote → kClosed
The DCHECK statements provide good debugging support for invalid transitions.
345-354: LGTM! Excellent API design improvement.The
CloseStreamArgsstruct makes stream closure behavior explicit and maintainable compared to multiple boolean parameters. This enhances code readability and reduces parameter passing errors.
358-386: LGTM! Well-reasoned keepalive integration.The wrapper methods ensure consistent keepalive notifications on all reads. The comment explaining why read failures are ignored for keepalive purposes shows good architectural thinking.
402-424: LGTM! Proper error categorization and handling.The error handling correctly distinguishes between stream errors (close stream, continue transport) and connection errors (close transport). This follows HTTP/2 error handling best practices.
437-453: LGTM! Comprehensive keepalive and ping management.The timeout duration members and
KeepaliveManagerprovide robust connection health monitoring. TheWaitForPingAckmethod complements the ping functionality well.deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc (7)
65-92: LGTM! Robust telemetry-driven rate measurement.The validation logic properly handles edge cases:
- Checks for negative bytes_per_nanosecond values
- Handles NaN values gracefully
- Uses rate-limited logging to prevent spam
- Updates tracking timestamps appropriately
This telemetry-based approach is more accurate than previous timing methods.
98-121: LGTM! Comprehensive delivery data calculation.The method properly handles edge cases:
- Fallback behavior when
current_rate <= 0(lines 115-120)- Careful time calculation to avoid rounding errors
- Reasonable fallback values for scheduling decisions
137-186: LGTM! Well-designed async reader with proper cancellation.Key strengths:
EndReadNexthandles cancellation gracefully by returning frames to output bufferPollReadNextuses proper waker management for async operationsSetNetworkMetricscorrectly triggers scheduler wakeup after updates- Latent tracing integration provides good observability
308-399: LGTM! Complex but well-structured scheduling algorithm.The multi-phase approach is well-designed:
- Data collection with proper locking
- Scheduler configuration with queued tokens
- Frame allocation based on delivery data
- Safe frame requeuing when readers unavailable
The extensive latent tracing provides excellent observability for performance debugging.
568-691: LGTM! Excellent telemetry integration design.The
MetricsCollectorclass provides robust telemetry integration:
- Proper handling of optional metric availability
- Comprehensive metric validation and conversion
- Clean integration with tracing and rate calculation
- Thread-safe event sink creation with proper lifetime management
The design properly abstracts event engine telemetry complexity.
745-858: LGTM! Clean endpoint loops with proper context management.Key strengths:
- Shared
EndpointContextprovides clean resource management- Frame serialization correctly handles padding and alignment requirements
- Metrics collection properly integrated in write path with rate limiting
- Clean separation between data and secure frame handling
- Proper error propagation with tracing integration
988-1021: LGTM! Proper channelz integration and lifecycle management.Excellent observability features:
- Constructor properly calls
SourceConstructed()for channelz lifecycleAddDataprovides comprehensive telemetry data from all components- Scheduler configuration parameter enhances flexibility
- Clean inheritance from
channelz::DataSourcedeps/grpc/src/core/BUILD (5)
1737-1760:thread_quotalinks tosyncbut notevent_engine_thread_pool
thread_quotaenforces limits that the thread-pool respects, but no dependency edge exists to guarantee consistent linkage. If both are linked into a binary with different ODR settings this can cause subtle counters to drift.Validate that whichever component instantiates
thread_quotaalso pulls inevent_engine_thread_pool, or add the dependency here.
6537-6546: Conflicting bit-gen settings can yield undefined RNGTwo config settings allow both
cpu_intensiveandmem_intensiveimplementations. If neither flag is set, the build falls back to the default but does not define either macro, leavingSharedBitGenbehaviour ambiguous.Add an
elsedefine such asGRPC_DEFAULT_BITGENor fail the configure step if neither is chosen.
6547-6562:shared_bit_genimplementation select relies solely on compile defsGood job adding compile-time selection via
defines = select({...}). 👍
9241-9249: Inconsistent queue implementationLegacy chaotic_good transport switched from
mpsctolock_based_mpsc, but the legacy variant still depends onlock_based_mpscwhile the non-legacy path uses plainmpsc. Verify that both sides of the handshake use the same queue to avoid ABA bugs.
556-565: Also applies to: 4008-4013, 5461-5468, 5579-5590, 6728-6736, 7367-7373deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc (5)
99-110: LGTM!Proper implementation of connectivity watch operations with appropriate mutex locking.
112-121: LGTM!Proper orphan handling with appropriate error status propagation and cleanup.
732-774: LGTM!Well-implemented idempotent stream closure with proper mutex locking, atomic state updates, and comprehensive cleanup logic.
776-843: LGTM!Excellent transport closure implementation with proper state tracking, stream cleanup, and asynchronous shutdown handling.
912-1014: LGTM!Sophisticated promise-based call handling implementation with proper stream lifecycle management, error handling, and cancellation support.
| config_setting( | ||
| name = "minimize_threadyness", | ||
| values = {"define": "GRPC_MINIMIZE_THREADYNESS=1"}, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Confirm mutual-exclusion policy between threadyness config flags
A new minimize_threadyness config_setting is introduced, but there is no guard to ensure it is not simultaneously selected with maximize_threadyness. Bazel will happily allow both defines to be passed if the user supplies both --define= flags, leading to an ambiguous compile-time state.
Consider adding a guard macro (or a single enum-valued define) so the two settings are mutually exclusive at build time.
🤖 Prompt for AI Agents
In deps/grpc/src/core/BUILD around lines 57 to 60, the new config_setting
"minimize_threadyness" lacks a mechanism to prevent it from being selected
simultaneously with "maximize_threadyness," which can cause conflicting
compile-time defines. To fix this, implement a mutual-exclusion guard by either
introducing a single enum-valued define that represents the threadyness mode or
adding a macro that ensures only one of these config_settings can be active at a
time during the build. This will prevent both flags from being passed together
and avoid ambiguous build states.
| grpc_cc_library( | ||
| name = "call_creds_registry", | ||
| hdrs = [ | ||
| "credentials/call/call_creds_registry.h", | ||
| ], | ||
| external_deps = ["absl/strings"], | ||
| deps = [ | ||
| "json", | ||
| "json_args", | ||
| "ref_counted", | ||
| "validation_errors", | ||
| "//:gpr_platform", | ||
| "//:ref_counted_ptr", | ||
| ], | ||
| ) | ||
|
|
There was a problem hiding this comment.
Visibility of call_creds_registry
call_creds_registry is consumed by //xds/... targets outside src/core, yet no visibility attribute is specified, so it inherits package-private visibility and will fail to build if those external packages are in different Bazel packages.
Add
visibility = ["//visibility:public"],or a narrower list that still includes the consuming packages.
🤖 Prompt for AI Agents
In deps/grpc/src/core/BUILD around lines 1623 to 1638, the grpc_cc_library
target "call_creds_registry" lacks a visibility attribute, causing build
failures when consumed by external packages like //xds/.... Add a visibility
attribute with either ["//visibility:public"] or a more restrictive list that
includes all consuming packages to ensure proper access across Bazel packages.
| grpc_cc_library( | ||
| name = "posix_event_engine_posix_interface", | ||
| srcs = [ | ||
| "lib/event_engine/posix_engine/posix_interface_posix.cc", | ||
| "lib/event_engine/posix_engine/posix_interface_windows.cc", | ||
| ], | ||
| hdrs = [ | ||
| "lib/event_engine/posix_engine/posix_interface.h", | ||
| ], | ||
| external_deps = [ | ||
| "absl/cleanup", | ||
| "absl/functional:any_invocable", | ||
| "absl/log:check", | ||
| "absl/log:log", | ||
| "absl/status", | ||
| "absl/strings", | ||
| "absl/strings:str_format", | ||
| ], | ||
| deps = [ | ||
| "event_engine_tcp_socket_utils", | ||
| "experiments", | ||
| "iomgr_port", | ||
| "posix_event_engine_file_descriptor_collection", | ||
| "posix_event_engine_tcp_socket_utils", | ||
| "status_helper", | ||
| "strerror", | ||
| "sync", | ||
| "//:event_engine_base_hdrs", | ||
| "//:gpr", | ||
| "//:gpr_platform", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
posix_event_engine_posix_interface builds both POSIX and Windows sources
posix_interface_posix.cc and posix_interface_windows.cc are compiled unconditionally.
On non-Windows builds, the Windows compilation unit drags in <winsock2.h> guards and vice-versa, causing macro clashes.
Add an appropriate select() on //conditions:windows vs default, or wrap the cc_library in platform-specific srcs.
🤖 Prompt for AI Agents
In deps/grpc/src/core/BUILD around lines 2219 to 2250, the grpc_cc_library
target posix_event_engine_posix_interface includes both posix_interface_posix.cc
and posix_interface_windows.cc unconditionally, causing macro clashes due to
conflicting platform headers. Fix this by using a select() statement on the
//conditions:windows condition to include posix_interface_windows.cc only on
Windows builds and posix_interface_posix.cc on non-Windows builds, ensuring
platform-specific source files are compiled appropriately.
| grpc_cc_library( | ||
| name = "chaotic_good_scheduler", | ||
| srcs = [ | ||
| "ext/transport/chaotic_good/scheduler.cc", | ||
| ], | ||
| hdrs = [ | ||
| "ext/transport/chaotic_good/scheduler.h", | ||
| ], | ||
| external_deps = [ | ||
| "absl/log", | ||
| "absl/strings", | ||
| ], | ||
| deps = [ | ||
| "chaotic_good_tcp_ztrace_collector", | ||
| "shared_bit_gen", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
chaotic_good_scheduler has no unit tests
Given the scheduler’s role in chaotic_good transports, a race condition would be disastrous. Add at least a basic stress test with multiple producers/consumers under TSAN in the test/ tree.
🤖 Prompt for AI Agents
In deps/grpc/src/core/BUILD around lines 8742 to 8758, the
chaotic_good_scheduler target lacks unit tests, which is critical given its role
in chaotic_good transports. Add a new test file in the test/ directory that
implements a basic stress test simulating multiple producers and consumers
running concurrently. Ensure the test is designed to run under ThreadSanitizer
(TSAN) to detect race conditions. Finally, update the BUILD file to include this
new test target linked to chaotic_good_scheduler.
| name = "ssl_transport_security_utils", | ||
| srcs = [ | ||
| "//src/core:tsi/ssl_transport_security_utils.cc", | ||
| ], | ||
| hdrs = [ | ||
| "//src/core:tsi/ssl_transport_security_utils.h", | ||
| ], | ||
| external_deps = [ | ||
| "absl/base:core_headers", | ||
| "absl/log:check", | ||
| "absl/log:log", | ||
| "absl/status", | ||
| "absl/status:statusor", | ||
| "absl/strings", | ||
| "libcrypto", | ||
| "libssl", | ||
| ], | ||
| deps = [ | ||
| "channel_args", | ||
| "env", | ||
| "error", | ||
| "grpc_transport_chttp2_alpn", | ||
| "load_file", | ||
| "ref_counted", | ||
| "slice", | ||
| "ssl_key_logging", | ||
| "sync", | ||
| "tsi_ssl_types", | ||
| "useful", | ||
| "//:channel_arg_names", | ||
| "//:config_vars", | ||
| "//:gpr", | ||
| "//:grpc_base", | ||
| "//:grpc_core_credentials_header", | ||
| "//:grpc_credentials_util", | ||
| "//:grpc_public_hdrs", | ||
| "//:grpc_security_base", | ||
| "//:ref_counted_ptr", | ||
| "//:transport_auth_context", | ||
| "//:tsi_base", | ||
| "//:tsi_ssl_session_cache", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
Security review required for ssl_transport_security_utils / ssl_key_logging
Key-logging support is compiled in by default and the libraries are public. Unless explicitly gated by a runtime env-var this can leak TLS master secrets in production.
Confirm that:
- Key logging is disabled unless
GRPC_SSL_KEY_LOG_FILE(or similar) is set; and - The feature is compiled out with
--define=GRPC_DISABLE_SSL_KEY_LOGGING=1for hardened builds.
If not, add compile-time and runtime guards.
🤖 Prompt for AI Agents
In deps/grpc/src/core/BUILD around lines 10218 to 10260, the
ssl_transport_security_utils target includes ssl_key_logging by default, which
risks leaking TLS master secrets. To fix this, add a compile-time guard that
excludes ssl_key_logging when the build is done with
--define=GRPC_DISABLE_SSL_KEY_LOGGING=1, and ensure the runtime code disables
key logging unless the environment variable GRPC_SSL_KEY_LOG_FILE is set. Modify
the BUILD file to conditionally include ssl_key_logging based on this define and
update the source code to check the env var before enabling key logging.
| uint16_t id; | ||
| if (next_free_entry_ != kSentinelId) { | ||
| id = next_free_entry_; | ||
| next_free_entry_ = entries_[id].next_chronologically; | ||
| } else { | ||
| id = entries_.size(); | ||
| DCHECK_NE(id, kSentinelId); | ||
| entries_.emplace_back(); |
There was a problem hiding this comment.
Potential ID overflow issue with uint16_t.
The code uses uint16_t for entry IDs, limiting the system to 65535 entries. If entries_.size() reaches 65535, the next ID would equal kSentinelId, causing undefined behavior.
Consider adding an overflow check:
} else {
id = entries_.size();
- DCHECK_NE(id, kSentinelId);
+ if (id == kSentinelId) {
+ // Handle overflow - either fail gracefully or use a larger type
+ LOG(ERROR) << "ChannelTrace entry ID overflow";
+ return EntryRef::Sentinel();
+ }
entries_.emplace_back();Alternatively, consider using uint32_t for IDs to support more entries.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint16_t id; | |
| if (next_free_entry_ != kSentinelId) { | |
| id = next_free_entry_; | |
| next_free_entry_ = entries_[id].next_chronologically; | |
| } else { | |
| id = entries_.size(); | |
| DCHECK_NE(id, kSentinelId); | |
| entries_.emplace_back(); | |
| uint16_t id; | |
| if (next_free_entry_ != kSentinelId) { | |
| id = next_free_entry_; | |
| next_free_entry_ = entries_[id].next_chronologically; | |
| } else { | |
| id = entries_.size(); | |
| if (id == kSentinelId) { | |
| // Handle overflow – either fail gracefully or use a larger type | |
| LOG(ERROR) << "ChannelTrace entry ID overflow"; | |
| return EntryRef::Sentinel(); | |
| } | |
| entries_.emplace_back(); |
🤖 Prompt for AI Agents
In deps/grpc/src/core/channelz/channel_trace.cc around lines 102 to 109, the use
of uint16_t for entry IDs risks overflow when entries_.size() reaches 65535,
causing the ID to equal kSentinelId and leading to undefined behavior. To fix
this, add a check before assigning id from entries_.size() to ensure it does not
exceed the maximum value for uint16_t, or change the type of id and related
variables from uint16_t to uint32_t to support a larger range of entries safely.
| void ChannelTrace::DropEntryId(uint16_t id) { | ||
| Entry& e = entries_[id]; | ||
| while (e.first_child != kSentinelId) { | ||
| DropEntryId(e.first_child); | ||
| } | ||
| if (e.prev_chronologically != kSentinelId) { | ||
| Entry& prev = entries_[e.prev_chronologically]; | ||
| DCHECK_EQ(prev.next_chronologically, id); | ||
| prev.next_chronologically = e.next_chronologically; | ||
| } | ||
| if (e.next_chronologically != kSentinelId) { | ||
| Entry& next = entries_[e.next_chronologically]; | ||
| DCHECK_EQ(next.prev_chronologically, id); | ||
| next.prev_chronologically = e.prev_chronologically; | ||
| } | ||
| if (e.prev_sibling != kSentinelId) { | ||
| Entry& prev = entries_[e.prev_sibling]; | ||
| DCHECK_EQ(prev.next_sibling, id); | ||
| prev.next_sibling = e.next_sibling; | ||
| } | ||
| if (e.next_sibling != kSentinelId) { | ||
| Entry& next = entries_[e.next_sibling]; | ||
| DCHECK_EQ(next.prev_sibling, id); | ||
| next.prev_sibling = e.prev_sibling; | ||
| } | ||
| if (e.parent != kSentinelId) { | ||
| Entry& p = entries_[e.parent]; | ||
| if (p.first_child == id) { | ||
| p.first_child = e.next_sibling; | ||
| } | ||
| if (p.last_child == id) { | ||
| p.last_child = e.prev_sibling; | ||
| } | ||
| } | ||
| // create and fill up the new event | ||
| AddTraceEventHelper( | ||
| new TraceEvent(severity, data, std::move(referenced_entity))); | ||
| if (first_entry_ == id) { | ||
| first_entry_ = e.next_chronologically; | ||
| } | ||
| if (last_entry_ == id) { | ||
| last_entry_ = e.prev_chronologically; | ||
| } | ||
| ++e.salt; | ||
| e.next_chronologically = next_free_entry_; | ||
| current_memory_ -= e.renderer->MemoryUsage(); | ||
| e.renderer.reset(); | ||
| DCHECK_EQ(current_memory_, MemoryUsageOf(entries_)); | ||
| next_free_entry_ = id; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider stack overflow protection in recursive deletion.
The DropEntryId method recursively deletes all children without depth limits. Deep hierarchies could cause stack overflow.
Consider adding a depth limit or converting to an iterative approach:
// Option 1: Add depth limit
void DropEntryId(uint16_t id, int depth = 0) {
if (depth > kMaxTraceDepth) {
LOG(ERROR) << "Trace depth exceeded, truncating deletion";
return;
}
// ... existing code ...
while (e.first_child != kSentinelId) {
DropEntryId(e.first_child, depth + 1);
}
}
// Option 2: Use iterative approach with stack
void DropEntryId(uint16_t id) {
std::stack<uint16_t> to_delete;
to_delete.push(id);
while (!to_delete.empty()) {
// Process entries iteratively
}
}🤖 Prompt for AI Agents
In deps/grpc/src/core/channelz/channel_trace.cc between lines 158 and 204, the
DropEntryId function uses recursion to delete child entries, which risks stack
overflow on deep hierarchies. To fix this, refactor DropEntryId to either
include a depth parameter with a maximum depth check that logs and stops
recursion when exceeded, or convert the function to an iterative approach using
an explicit stack to manage entries to delete, thereby avoiding deep recursive
calls.
| void PropertyGrid::SetInternal(absl::string_view column, absl::string_view row, | ||
| std::optional<PropertyValue> value) { | ||
| int c = GetIndex(columns_, column); | ||
| int r = GetIndex(rows_, row); | ||
| if (value.has_value()) { | ||
| grid_.emplace(std::pair(c, r), *std::move(value)); | ||
| } else { | ||
| grid_.erase(std::pair(c, r)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Use size_t consistently for index types
The function uses int for indices returned by GetIndex but stores them in pairs with size_t. This inconsistency could cause issues with signed/unsigned comparisons.
void PropertyGrid::SetInternal(absl::string_view column, absl::string_view row,
std::optional<PropertyValue> value) {
- int c = GetIndex(columns_, column);
- int r = GetIndex(rows_, row);
+ size_t c = GetIndex(columns_, column);
+ size_t r = GetIndex(rows_, row);
if (value.has_value()) {
grid_.emplace(std::pair(c, r), *std::move(value));
} else {
grid_.erase(std::pair(c, r));
}
}Similar changes needed in:
- Lines 235-236 (PropertyGrid::SetInternal)
- Lines 246, 253-254 (PropertyGrid::SetColumn)
- Lines 260, 267-268 (PropertyGrid::SetRow)
- Line 300 (PropertyTable::SetInternal)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void PropertyGrid::SetInternal(absl::string_view column, absl::string_view row, | |
| std::optional<PropertyValue> value) { | |
| int c = GetIndex(columns_, column); | |
| int r = GetIndex(rows_, row); | |
| if (value.has_value()) { | |
| grid_.emplace(std::pair(c, r), *std::move(value)); | |
| } else { | |
| grid_.erase(std::pair(c, r)); | |
| } | |
| } | |
| void PropertyGrid::SetInternal(absl::string_view column, absl::string_view row, | |
| std::optional<PropertyValue> value) { | |
| size_t c = GetIndex(columns_, column); | |
| size_t r = GetIndex(rows_, row); | |
| if (value.has_value()) { | |
| grid_.emplace(std::pair(c, r), *std::move(value)); | |
| } else { | |
| grid_.erase(std::pair(c, r)); | |
| } | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/channelz/property_list.cc around lines 233 to 242, change
the index variables c and r from int to size_t to match the type used in the
grid_ map keys. Similarly, update the index variable types in
PropertyGrid::SetColumn (lines 246, 253-254), PropertyGrid::SetRow (lines 260,
267-268), and PropertyTable::SetInternal (line 300) to use size_t instead of int
for consistency and to avoid signed/unsigned comparison issues.
| if (stream_id > RFC9113::kMaxStreamId31Bit) { | ||
| // TODO(tjagtap) : [PH2][P3] : Handle case if transport runs out of stream | ||
| // ids | ||
| // RFC9113 : Stream identifiers cannot be reused. Long-lived connections | ||
| // can result in an endpoint exhausting the available range of stream | ||
| // identifiers. A client that is unable to establish a new stream | ||
| // identifier can establish a new connection for new streams. A server | ||
| // that is unable to establish a new stream identifier can send a GOAWAY | ||
| // frame so that the client is forced to open a new connection for new | ||
| // streams. | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Address stream ID exhaustion handling before production use.
The TODO indicates RFC9113 compliance is needed - stream ID exhaustion should trigger connection closure with GOAWAY frame for servers or new connection establishment for clients. This is critical for long-lived connections.
Implement RFC9113 Stream-ID Exhaustion Handling
The empty TODO at deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h:308–318 must be replaced with real logic to avoid leaking a long-lived connection once stream IDs run out. Per RFC9113:
• Clients: detect stream_id > kMaxStreamId31Bit, tear down the existing HTTP/2 session, and open a new connection for further streams.
• Servers: send a GOAWAY frame to force clients to establish a fresh connection for new streams.
Please update the if (stream_id > RFC9113::kMaxStreamId31Bit) block to:
- Close or reset the current transport.
- For servers, emit a GOAWAY (e.g., via
grpc_chttp2_initiate_goaway). - For clients, signal the caller to reconnect (or automatically spin up a new channel).
- Add unit tests covering the exhaustion path.
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h
around lines 308 to 318, replace the TODO block handling stream_id exhaustion
with actual logic per RFC9113. Implement detection of stream_id exceeding
kMaxStreamId31Bit, then for servers send a GOAWAY frame using
grpc_chttp2_initiate_goaway to prompt clients to reconnect, and for clients
close or reset the current transport and signal or initiate a new connection.
Also add unit tests to cover this stream ID exhaustion scenario.
f14b099 to
84b2296
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (24)
deps/grpc/Makefile (2)
89-90: Leading dash still missing – linker flag is ignored
LDFLAGS_asan-noleakscontinues to readfsanitize=address; without the leading-, the flag is taken as a filename and ASan is silently disabled (or the build fails looking for that file).
The patch proposed in the previous review is still required:-LDFLAGS_asan-noleaks = fsanitize=address +LDFLAGS_asan-noleaks = -fsanitize=address
407-408: Filename / SONAME mismatch persists – runtime loading will break
SHARED_VERSION_COREretains the leading hyphen, producinglibgrpc-49.so.49.0.0, while the-sonameislibgrpc.so.49. Any binary linked againstlibgrpc.so.49will fail at load-time because no file with that SONAME exists.
The minimal fix from the earlier review is still outstanding; please remove the leading hyphen or adjust the SONAME/symlink logic consistently.Also applies to: 864-866
deps/grpc/src/core/ext/transport/chttp2/client/chttp2_connector.cc (1)
209-211: Fix error handling when taking endpoint fails.The code logs an error but then continues to use the null
event_engine_endpoint, which will likely cause a crash. The error should be propagated properly.- LOG(ERROR) << "Failed to take endpoint."; - result = GRPC_ERROR_CREATE("Failed to take endpoint."); + result = GRPC_ERROR_CREATE("Failed to take endpoint."); + result_->Reset(); + NullThenSchedClosure(DEBUG_LOCATION, ¬ify_, result.status()); + return;deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc (2)
659-661: Documented Python compatibility check remains valid.The
EventEngineExperimentDisabledForPython()check is still present and properly documented from previous reviews. The implementation correctly falls back when the experimental engine is disabled for Python.The Python experimental Event Engine flag continues to provide necessary compatibility fallback as previously documented.
223-272: Critical issue persists: CHECK macro crashes process on failure.The
FdHandler::Handlemethod still usesCHECKat line 242-244, which will crash the process ifHandleExternalConnectionfails. This is the same issue flagged in previous reviews.The use of
CHECKwill terminate the process on failure. External connection handling should use graceful error handling instead:- CHECK(GRPC_LOG_IF_ERROR("listener_handle_external_connection", - listener_supports_fd_->HandleExternalConnection( - listener_fd, fd, &pending_data))); + auto status = listener_supports_fd_->HandleExternalConnection( + listener_fd, fd, &pending_data); + if (!status.ok()) { + LOG(ERROR) << "Failed to handle external connection: " << status; + // TODO: Consider closing fd or other cleanup + }deps/grpc/src/core/ext/transport/chttp2/transport/header_assembler.h (3)
98-130: DCHECK still used for external input validation.Line 100-101 still uses
DCHECK_GTto validateframe.stream_id, which only validates in debug builds. Since this validates HTTP/2 frames from peers (external input), runtime validation should be used in all builds.Replace the DCHECK with runtime validation:
- DCHECK_GT(frame.stream_id, 0u) - << "RFC9113 : HEADERS frames MUST be associated with a stream."; + if (frame.stream_id == 0) { + Cleanup(); + LOG(ERROR) << "RFC9113 : HEADERS frames MUST be associated with a stream."; + return Http2Status::Http2ConnectionError( + Http2ErrorCode::kProtocolError, + "HEADERS frames MUST be associated with a stream."); + }
134-147: Missing critical validations in AppendContinuationFrame.The
AppendContinuationFramemethod still lacks the important validations present inAppendHeaderFrame, as previously flagged.Add the missing validation checks:
Http2Status AppendContinuationFrame(Http2ContinuationFrame&& frame) { + // Validate header is in progress + if (!header_in_progress_) { + Cleanup(); + LOG(ERROR) << kAssemblerContiguousSequenceError; + return Http2Status::Http2ConnectionError( + Http2ErrorCode::kProtocolError, + std::string(kAssemblerContiguousSequenceError)); + } + + // Validate stream ID matches + if (frame.stream_id != stream_id_) { + Cleanup(); + LOG(ERROR) << kAssemblerMismatchedStreamId; + return Http2Status::Http2ConnectionError( + Http2ErrorCode::kProtocolError, + std::string(kAssemblerMismatchedStreamId)); + } + + // Check for overflow on 32-bit platforms + const size_t current_len = frame.payload.Length(); + if constexpr (sizeof(size_t) == 4) { + if (GPR_UNLIKELY(buffer_.Length() >= UINT32_MAX - current_len)) { + Cleanup(); + LOG(ERROR) + << "Stream Error: SliceBuffer overflow for 32 bit platforms."; + return Http2Status::Http2StreamError( + Http2ErrorCode::kInternalError, + "Stream Error: SliceBuffer overflow for 32 bit platforms."); + } + } + // Manage payload - const size_t current_len = frame.payload.Length(); frame.payload.MoveFirstNBytesIntoSliceBuffer(current_len, buffer_);
165-169: Metadata size limits still not properly implemented.Lines 167-168 still set both soft and hard limits to
std::numeric_limits<uint32_t>::max(), which effectively disables size enforcement and could lead to memory exhaustion attacks.Implement proper configurable metadata size limits instead of using maximum values that disable enforcement.
deps/grpc/src/core/channelz/channel_trace.cc (2)
102-109: ID overflow issue remains unaddressed.The code still uses
uint16_tfor entry IDs, limiting the system to 65535 entries. Whenentries_.size()reaches 65535, the next ID would equalkSentinelId, causing undefined behavior as flagged in previous reviews.Consider adding the overflow check as previously suggested:
} else { id = entries_.size(); - DCHECK_NE(id, kSentinelId); + if (id == kSentinelId) { + // Handle overflow - either fail gracefully or use a larger type + LOG(ERROR) << "ChannelTrace entry ID overflow"; + return EntryRef::Sentinel(); + } entries_.emplace_back();
158-204: Recursive deletion still risks stack overflow.The
DropEntryIdmethod still uses unlimited recursion to delete child entries, which could cause stack overflow on deep hierarchies as previously identified.The suggested iterative approach or depth limiting from the previous review should still be implemented:
// Option 1: Add depth limit void DropEntryId(uint16_t id, int depth = 0) { if (depth > kMaxTraceDepth) { LOG(ERROR) << "Trace depth exceeded, truncating deletion"; return; } // ... existing code ... while (e.first_child != kSentinelId) { DropEntryId(e.first_child, depth + 1); } }deps/grpc/src/core/channelz/channelz.h (1)
130-147: EntityTypeToKind switch still missing default case.The switch statement still lacks a default case or unreachable annotation, which could cause undefined behavior if the enum is extended, as previously flagged.
static absl::string_view EntityTypeToKind(EntityType type) { switch (type) { case EntityType::kTopLevelChannel: return "channel"; case EntityType::kInternalChannel: return "internal_channel"; case EntityType::kSubchannel: return "subchannel"; case EntityType::kServer: return "server"; case EntityType::kListenSocket: return "listen_socket"; case EntityType::kSocket: return "socket"; case EntityType::kCall: return "call"; } + GPR_UNREACHABLE_CODE(return "unknown"); }deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h (1)
117-127: Use release ordering for scheduling_state_ stores to match the destructor's acquire loadThe destructor loads
scheduling_state_withmemory_order_acquire, but stores in the implementation file may still usememory_order_relaxed. Without matching release ordering on stores, there's no happens-before relationship to guarantee the Waker pointer is fully published before deletion.Ensure all
scheduling_state_.store(...)calls in data_endpoints.cc use at leaststd::memory_order_releaseto properly synchronize with the destructor'sload(std::memory_order_acquire).deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h (2)
308-318: Implement RFC9113 stream ID exhaustion handlingThe TODO block must be replaced with actual logic to handle stream ID exhaustion per RFC9113. When
stream_id > RFC9113::kMaxStreamId31Bit, the transport should close the existing HTTP/2 session and establish a new connection.
527-534: Address transport lifetime management in interface implementationsBoth
PingSystemInterfaceImplandKeepAliveInterfaceImplstore raw pointers to the transport, which could become dangling if the transport is destroyed while these interfaces are still in use. The TODO comments acknowledge this issue.Consider using
RefCountedPtror implementing a separate ref-counted structure to ensure proper lifetime management.Also applies to: 575-581
deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc (1)
274-275: Avoid LOG(FATAL) in production codeUsing
LOG(FATAL)will crash the process. Consider returning an error or usingLOG(DFATAL)for debug builds only.- LOG(FATAL) << "Unreachable state: " << state; + LOG(DFATAL) << "Unreachable state: " << state; + return Pending{}; // Defensive fallbackAlso applies to: 304-305
deps/grpc/src/core/BUILD (3)
57-60: Still no guard betweenmax/minimize_threadynessflagsThe newly-added
minimize_threadynessconfig_settingcan still be selected together withmaximize_threadyness, leaving the build with conflicting defines. Introduce a mutually-exclusive mechanism (e.g. single enum-valued define or validation macro) to prevent both flags from being active simultaneously.
1623-1637:call_creds_registrytarget lacksvisibilityThis library is consumed by
//xds/...targets outsidesrc/core, but inherits package-private visibility. Add an explicitvisibilityattribute (public or appropriately scoped) to avoid downstream build failures.
2219-2250: Platform sources compiled unconditionally
posix_event_engine_posix_interfacelists bothposix_interface_posix.ccandposix_interface_windows.ccwithout aselect()on platform constraints, causing header and macro clashes. Gate the sources with//conditions:windowsvs default (or split into two platform-specific libraries).deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc (6)
86-87: Replace DCHECK with proper error handling for unimplemented operations.The DCHECK is only active in debug builds and won't catch unimplemented transport operations in production, potentially causing silent failures.
217-229: Incomplete stream validation may cause protocol violations.The header frame processing returns
Ok()when stream lookup fails, but the comment indicates this should be a PROTOCOL_ERROR per RFC 9113.
318-330: Settings frame content is not processed.The function acknowledges settings receipt but doesn't actually apply the settings values, which could lead to protocol violations.
371-379: GOAWAY frame processing is not implemented.The GOAWAY frame handler has a TODO but returns
Ok(). This could cause the client to continue using a connection that the server is shutting down.
714-718: Handle failure from Apply when setting initial window sizeThe call to
settings_.mutable_local().Apply(...)usesDCHECK, which is stripped out in release builds. A runtime check is needed to catch and handle failures in production.
869-870: Add stream ID validation per RFC 9113.Stream identifiers must be validated according to RFC 9113 requirements.
🧹 Nitpick comments (1)
deps/grpc/src/core/channelz/channelz.cc (1)
248-260: Consider graceful error handling instead of LOG(DFATAL)The
SourceDestructing()method usesLOG(DFATAL)when a DataSource is not found in the node's data sources vector. While this helps catch programming errors in debug builds, it could be problematic in production.Consider using
LOG(ERROR)with additional diagnostics to help debug the issue without risking a crash:- LOG(DFATAL) << "DataSource not found in node's data sources -- probably " - "SourceConstructed was not called"; + LOG(ERROR) << "DataSource not found in node's data sources (node=" + << node->uuid() << ", type=" << node->type_ + << ") -- probably SourceConstructed was not called"; + DCHECK(false) << "DataSource lifecycle error";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (154)
deps/grpc/.bazelci/presubmit.yml(1 hunks)deps/grpc/Makefile(22 hunks)deps/grpc/grpc.gyp(11 hunks)deps/grpc/include/grpc/create_channel_from_endpoint.h(1 hunks)deps/grpc/include/grpc/credentials.h(1 hunks)deps/grpc/include/grpc/event_engine/event_engine.h(7 hunks)deps/grpc/include/grpc/grpc_posix.h(1 hunks)deps/grpc/include/grpc/impl/channel_arg_names.h(1 hunks)deps/grpc/include/grpc/module.modulemap(1 hunks)deps/grpc/include/grpc/support/json.h(1 hunks)deps/grpc/include/grpcpp/channel.h(2 hunks)deps/grpc/include/grpcpp/create_channel_posix.h(2 hunks)deps/grpc/include/grpcpp/impl/generic_serialize.h(3 hunks)deps/grpc/include/grpcpp/security/credentials.h(2 hunks)deps/grpc/include/grpcpp/support/callback_common.h(2 hunks)deps/grpc/include/grpcpp/support/client_callback.h(3 hunks)deps/grpc/include/grpcpp/version_info.h(1 hunks)deps/grpc/src/compiler/php_generator.cc(1 hunks)deps/grpc/src/core/BUILD(96 hunks)deps/grpc/src/core/call/filter_fusion.h(2 hunks)deps/grpc/src/core/call/interception_chain.h(4 hunks)deps/grpc/src/core/channelz/channel_trace.cc(1 hunks)deps/grpc/src/core/channelz/channel_trace.h(3 hunks)deps/grpc/src/core/channelz/channelz.cc(20 hunks)deps/grpc/src/core/channelz/channelz.h(20 hunks)deps/grpc/src/core/channelz/channelz_registry.cc(1 hunks)deps/grpc/src/core/channelz/channelz_registry.h(3 hunks)deps/grpc/src/core/channelz/property_list.cc(1 hunks)deps/grpc/src/core/channelz/property_list.h(1 hunks)deps/grpc/src/core/channelz/ztrace_collector.h(3 hunks)deps/grpc/src/core/channelz/zviz/data.cc(1 hunks)deps/grpc/src/core/channelz/zviz/data.h(1 hunks)deps/grpc/src/core/channelz/zviz/entity.cc(1 hunks)deps/grpc/src/core/channelz/zviz/entity.h(1 hunks)deps/grpc/src/core/channelz/zviz/environment.cc(1 hunks)deps/grpc/src/core/channelz/zviz/environment.h(1 hunks)deps/grpc/src/core/channelz/zviz/html.cc(1 hunks)deps/grpc/src/core/channelz/zviz/html.h(1 hunks)deps/grpc/src/core/channelz/zviz/layout.cc(1 hunks)deps/grpc/src/core/channelz/zviz/layout.h(1 hunks)deps/grpc/src/core/channelz/zviz/layout_html.cc(1 hunks)deps/grpc/src/core/channelz/zviz/layout_html.h(1 hunks)deps/grpc/src/core/channelz/zviz/strings.cc(1 hunks)deps/grpc/src/core/channelz/zviz/strings.h(1 hunks)deps/grpc/src/core/channelz/zviz/trace.cc(1 hunks)deps/grpc/src/core/channelz/zviz/trace.h(1 hunks)deps/grpc/src/core/client_channel/client_channel.cc(5 hunks)deps/grpc/src/core/client_channel/client_channel_filter.cc(4 hunks)deps/grpc/src/core/client_channel/config_selector.h(2 hunks)deps/grpc/src/core/client_channel/dynamic_filters.cc(2 hunks)deps/grpc/src/core/client_channel/dynamic_filters.h(1 hunks)deps/grpc/src/core/client_channel/global_subchannel_pool.cc(1 hunks)deps/grpc/src/core/client_channel/retry_filter.cc(1 hunks)deps/grpc/src/core/client_channel/retry_filter.h(4 hunks)deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.cc(3 hunks)deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.h(1 hunks)deps/grpc/src/core/client_channel/retry_interceptor.cc(5 hunks)deps/grpc/src/core/client_channel/retry_interceptor.h(3 hunks)deps/grpc/src/core/client_channel/retry_throttle.cc(3 hunks)deps/grpc/src/core/client_channel/retry_throttle.h(1 hunks)deps/grpc/src/core/client_channel/subchannel.cc(6 hunks)deps/grpc/src/core/client_channel/subchannel.h(2 hunks)deps/grpc/src/core/config/config_vars.cc(1 hunks)deps/grpc/src/core/config/config_vars.yaml(1 hunks)deps/grpc/src/core/config/core_configuration.cc(1 hunks)deps/grpc/src/core/config/core_configuration.h(5 hunks)deps/grpc/src/core/credentials/call/call_creds_registry.h(1 hunks)deps/grpc/src/core/credentials/call/call_creds_registry_init.cc(1 hunks)deps/grpc/src/core/credentials/call/gcp_service_account_identity/gcp_service_account_identity_credentials.cc(2 hunks)deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.cc(1 hunks)deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.h(1 hunks)deps/grpc/src/core/credentials/call/jwt_util.cc(1 hunks)deps/grpc/src/core/credentials/call/jwt_util.h(1 hunks)deps/grpc/src/core/credentials/transport/channel_creds_registry_init.cc(1 hunks)deps/grpc/src/core/credentials/transport/google_default/google_default_credentials.cc(4 hunks)deps/grpc/src/core/credentials/transport/ssl/ssl_credentials.cc(0 hunks)deps/grpc/src/core/credentials/transport/tls/load_system_roots_supported.cc(1 hunks)deps/grpc/src/core/credentials/transport/xds/xds_credentials.cc(0 hunks)deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc(1 hunks)deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h(1 hunks)deps/grpc/src/core/ext/filters/http/client_authority_filter.cc(1 hunks)deps/grpc/src/core/ext/filters/http/message_compress/compression_filter.h(4 hunks)deps/grpc/src/core/ext/filters/http/server/http_server_filter.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/client_transport.cc(6 hunks)deps/grpc/src/core/ext/transport/chaotic_good/client_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/config.h(4 hunks)deps/grpc/src/core/ext/transport/chaotic_good/control_endpoint.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc(6 hunks)deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h(5 hunks)deps/grpc/src/core/ext/transport/chaotic_good/frame.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/frame_transport.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/message_chunker.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/scheduler.cc(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/scheduler.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc(11 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h(5 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server_transport.cc(5 hunks)deps/grpc/src/core/ext/transport/chaotic_good/server_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_header.h(1 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.cc(7 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good/tcp_ztrace_collector.h(3 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/chaotic_good_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/client_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/server/chaotic_good_server.cc(2 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.cc(4 hunks)deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/client/chttp2_connector.cc(7 hunks)deps/grpc/src/core/ext/transport/chttp2/server/chttp2_server.cc(4 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.cc(17 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.h(0 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/flow_control.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame_data.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame_settings.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/frame_window_update.cc(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/header_assembler.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc(5 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.h(4 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc(15 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.h(11 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.cc(10 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_status.h(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_transport.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/http2_ztrace_collector.h(0 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/internal.h(6 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.cc(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.h(4 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/message_assembler.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/parsing.cc(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_callbacks.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.cc(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.h(0 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.h(2 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/stream_data_queue.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/stream_lists.cc(3 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.cc(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.h(1 hunks)deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc(10 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.c(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.c(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.h(1 hunks)deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/property_list.upb.h(1 hunks)
💤 Files with no reviewable changes (5)
- deps/grpc/src/core/credentials/transport/xds/xds_credentials.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.h
- deps/grpc/src/core/credentials/transport/ssl/ssl_credentials.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_ztrace_collector.h
✅ Files skipped from review due to trivial changes (8)
- deps/grpc/src/core/ext/transport/chttp2/transport/stream_data_queue.h
- deps/grpc/src/core/channelz/zviz/strings.h
- deps/grpc/include/grpcpp/version_info.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.h
- deps/grpc/src/core/channelz/zviz/layout.cc
- deps/grpc/src/core/channelz/zviz/strings.cc
- deps/grpc/.bazelci/presubmit.yml
- deps/grpc/src/core/credentials/call/call_creds_registry_init.cc
🚧 Files skipped from review as they are similar to previous changes (122)
- deps/grpc/src/core/channelz/channelz_registry.cc
- deps/grpc/src/core/credentials/transport/channel_creds_registry_init.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/frame_settings.cc
- deps/grpc/src/core/client_channel/dynamic_filters.h
- deps/grpc/src/core/credentials/transport/tls/load_system_roots_supported.cc
- deps/grpc/include/grpc/credentials.h
- deps/grpc/src/core/ext/transport/chttp2/transport/frame_data.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_transport.h
- deps/grpc/src/core/ext/transport/chaotic_good/config.h
- deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.cc
- deps/grpc/src/compiler/php_generator.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_stats_collector.cc
- deps/grpc/src/core/ext/transport/chttp2/server/chttp2_server.cc
- deps/grpc/src/core/ext/filters/http/server/http_server_filter.h
- deps/grpc/src/core/ext/transport/chttp2/transport/frame_window_update.cc
- deps/grpc/src/core/channelz/zviz/environment.h
- deps/grpc/include/grpcpp/channel.h
- deps/grpc/src/core/ext/filters/gcp_authentication/gcp_authentication_filter.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_status.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_promise.cc
- deps/grpc/src/core/channelz/zviz/layout_html.h
- deps/grpc/src/core/channelz/zviz/entity.h
- deps/grpc/src/core/ext/transport/chttp2/transport/stream_lists.cc
- deps/grpc/include/grpcpp/security/credentials.h
- deps/grpc/src/core/ext/transport/chttp2/transport/flow_control.h
- deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.h
- deps/grpc/src/core/channelz/ztrace_collector.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.cc
- deps/grpc/src/core/ext/transport/chaotic_good/message_chunker.h
- deps/grpc/src/core/call/interception_chain.h
- deps/grpc/src/core/channelz/zviz/environment.cc
- deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.h
- deps/grpc/src/core/client_channel/subchannel.h
- deps/grpc/include/grpcpp/support/callback_common.h
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/server/chaotic_good_server.cc
- deps/grpc/include/grpc/create_channel_from_endpoint.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_callbacks.h
- deps/grpc/src/core/client_channel/config_selector.h
- deps/grpc/src/core/ext/transport/chttp2/transport/ping_rate_policy.h
- deps/grpc/src/core/ext/transport/chaotic_good/server_transport.h
- deps/grpc/src/core/credentials/call/jwt_util.cc
- deps/grpc/include/grpcpp/impl/generic_serialize.h
- deps/grpc/src/core/credentials/call/gcp_service_account_identity/gcp_service_account_identity_credentials.cc
- deps/grpc/src/core/client_channel/retry_filter.h
- deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.h
- deps/grpc/include/grpc/module.modulemap
- deps/grpc/src/core/ext/transport/chaotic_good/client/chaotic_good_connector.cc
- deps/grpc/src/core/config/core_configuration.h
- deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_transport.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.cc
- deps/grpc/include/grpc/impl/channel_arg_names.h
- deps/grpc/src/core/ext/transport/chaotic_good/tcp_frame_header.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.c
- deps/grpc/src/core/credentials/call/jwt_util.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb.h
- deps/grpc/src/core/channelz/zviz/html.cc
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/client_transport.h
- deps/grpc/src/core/ext/transport/chttp2/transport/transport_common.h
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.cc
- deps/grpc/src/core/ext/transport/chaotic_good/frame.h
- deps/grpc/src/core/client_channel/client_channel.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/message_assembler.h
- deps/grpc/src/core/ext/transport/chttp2/transport/internal.h
- deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser.cc
- deps/grpc/src/core/ext/transport/chaotic_good/frame_transport.h
- deps/grpc/src/core/channelz/zviz/layout.h
- deps/grpc/src/core/ext/filters/http/client_authority_filter.cc
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/channelz.upb_minitable.h
- deps/grpc/src/core/config/core_configuration.cc
- deps/grpc/src/core/client_channel/global_subchannel_pool.cc
- deps/grpc/src/core/channelz/zviz/layout_html.cc
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/server_transport.h
- deps/grpc/src/core/ext/transport/chaotic_good/server_transport.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.cc
- deps/grpc/src/core/channelz/channelz_registry.h
- deps/grpc/src/core/credentials/transport/google_default/google_default_credentials.cc
- deps/grpc/src/core/client_channel/client_channel_filter.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/frame.h
- deps/grpc/src/core/config/config_vars.yaml
- deps/grpc/include/grpc/grpc_posix.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.c
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.cc
- deps/grpc/src/core/ext/transport/chaotic_good/control_endpoint.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_server_transport.h
- deps/grpc/include/grpc/support/json.h
- deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.cc
- deps/grpc/src/core/ext/transport/chaotic_good_legacy/chaotic_good_transport.h
- deps/grpc/src/core/credentials/call/call_creds_registry.h
- deps/grpc/src/core/ext/transport/chttp2/transport/parsing.cc
- deps/grpc/src/core/channelz/zviz/data.h
- deps/grpc/src/core/client_channel/retry_interceptor.h
- deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.cc
- deps/grpc/src/core/client_channel/retry_filter_legacy_call_data.cc
- deps/grpc/src/core/channelz/zviz/trace.cc
- deps/grpc/src/core/client_channel/subchannel.cc
- deps/grpc/src/core/ext/transport/chttp2/transport/hpack_parser_table.h
- deps/grpc/src/core/channelz/zviz/html.h
- deps/grpc/src/core/ext/transport/chttp2/transport/writing.cc
- deps/grpc/src/core/channelz/zviz/data.cc
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb_minitable.h
- deps/grpc/include/grpcpp/create_channel_posix.h
- deps/grpc/src/core/ext/transport/chttp2/transport/http2_settings.h
- deps/grpc/src/core/ext/transport/chaotic_good/client_transport.h
- deps/grpc/src/core/ext/transport/chttp2/transport/keepalive.h
- deps/grpc/src/core/ext/transport/chttp2/transport/frame.cc
- deps/grpc/src/core/channelz/zviz/trace.h
- deps/grpc/src/core/ext/transport/chaotic_good/scheduler.h
- deps/grpc/src/core/credentials/call/jwt_token_file/jwt_token_file_call_credentials.h
- deps/grpc/src/core/channelz/property_list.cc
- deps/grpc/src/core/ext/transport/chaotic_good/scheduler.cc
- deps/grpc/src/core/client_channel/retry_throttle.h
- deps/grpc/src/core/config/config_vars.cc
- deps/grpc/src/core/ext/filters/http/message_compress/compression_filter.h
- deps/grpc/src/core/channelz/zviz/entity.cc
- deps/grpc/src/core/call/filter_fusion.h
- deps/grpc/src/core/ext/transport/chaotic_good/client_transport.cc
- deps/grpc/src/core/client_channel/dynamic_filters.cc
- deps/grpc/include/grpcpp/support/client_callback.h
- deps/grpc/src/core/client_channel/retry_throttle.cc
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/property_list.upb.h
- deps/grpc/src/core/ext/upb-gen/src/proto/grpc/channelz/v2/promise.upb.h
- deps/grpc/src/core/ext/transport/chttp2/transport/chttp2_transport.cc
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: the `span` class in `doc/api/diagnostics_channel.md` is unrelated to the one in `lib/internal/otel/t...
Learnt from: santigimeno
PR: nodesource/nsolid#289
File: lib/internal/otel/trace.js:40-40
Timestamp: 2025-04-16T06:58:59.313Z
Learning: The `Span` class in `doc/api/diagnostics_channel.md` is unrelated to the one in `lib/internal/otel/trace.js`. They share the same name but serve completely different purposes - one is for documentation examples, while the other is part of the OpenTelemetry implementation.
Applied to files:
deps/grpc/src/core/channelz/channel_trace.h
📚 Learning: in the nsolid codebase, test files in test/addons/ consistently use assert() for input validation an...
Learnt from: santigimeno
PR: nodesource/nsolid#327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
Applied to files:
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: test-tarball-linux
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: test-macOS
- GitHub Check: lint-js-and-md
🔇 Additional comments (67)
deps/grpc/Makefile (2)
80-81: Sanitizer flag rewrite drops-DGPR_NO_DIRECT_SYSCALLS– verify side-effectsAll sanitizer‐specific
CPPFLAGS_*previously carried-DGPR_NO_DIRECT_SYSCALLS; the refresh removed it. That macro prevents direct low-level syscalls that sanitizers cannot intercept. Losing the define can surface false positives (MSan) or runtime aborts (TSan).Please confirm this was an intentional upstream change and that the test matrix (especially
make CONFIG=msan/tsan) still passes.Also applies to: 89-90, 98-99, 159-161, 186-188, 196-198
370-371: Version bump implies new ABI → ensure downstream packaging picks upso.49Incrementing
CORE_VERSIONto49.0.0rightfully moves the SONAME from 48→49. Double-check that distro packaging scripts (RPM/DEB) and any external CIs expectinglibgrpc.so.48are updated, otherwise upgrades will break at runtime.deps/grpc/grpc.gyp (6)
150-159: LGTM! Channelz enhancements and property list functionality added.The additions include the new
property_list.ccand corresponding protobuf generated files for channelz v2 functionality. The commented out zviz files suggest planned visualization features for future releases.
178-179: LGTM! JWT token file credentials functionality added.The addition of JWT token file credentials support with
jwt_token_file_call_credentials.ccandjwt_util.ccenhances the authentication capabilities.
263-274: LGTM! HTTP/2 transport enhancements added.New HTTP/2 client transport implementation files have been added, including statistics collection and transport common functionality. These support the modernized HTTP/2 transport layer mentioned in the AI summary.
406-408: LGTM! Channelz v2 protobuf bindings added.The protobuf-generated minitable and upbdefs files for channelz v2, promise, and property_list are correctly included in the build configuration.
Also applies to: 571-572
636-636: LGTM! Event engine enhancements and refactoring.The event engine component has been enhanced with new source files and the removal of
forkable.cc, indicating a refactoring of the event engine architecture. The new files support improved endpoint handling and POSIX interface management.Also applies to: 716-716, 768-768, 776-778
872-872: LGTM! Telemetry and utility additions.The addition of
context_list_entry.ccfor telemetry andwait_for_single_owner.ccfor utilities aligns with the expanded functionality described in the update.Also applies to: 971-971
deps/grpc/src/core/client_channel/retry_filter.cc (4)
95-112: LGTM! Improved retry throttling architecture with blackboard integration.The new
UpdateBlackboardmethod centralizes retry throttling configuration by:
- Extracting retry throttling parameters from service config
- Reusing existing throttlers from old blackboard when available
- Creating new throttlers with updated parameters
- Managing throttlers as blackboard entries
This is a cleaner architectural approach compared to direct channel args parsing.
114-122: LGTM! Simplified constructor using blackboard-based initialization.The new constructor properly initializes all members from the channel args and retrieves the retry throttler from the blackboard, eliminating the need for complex URI parsing and server-specific throttling data retrieval.
95-112: LGTM! Well-structured blackboard-based throttler management.The
UpdateBlackboardmethod correctly handles the lifecycle of retry throttlers through blackboard objects. The logic properly reuses existing throttlers when available and creates new ones with updated configuration, ensuring efficient resource management.
114-121: Empty-string blackboard key for RetryThrottler is consistentVerified that both
retry_filter.ccandretry_interceptor.ccuse
Get<internal::RetryThrottler>("")Set("", std::move(throttler))No inconsistencies found—LGTM!
deps/grpc/src/core/ext/transport/chttp2/client/chttp2_connector.cc (4)
21-21: LGTM! Enhanced imports for event engine and promise-based transport support.The new imports properly support:
- Event engine endpoint handling
- Promise-based HTTP/2 client transport (with conditional compilation)
- Endpoint channel arg wrapper functionality
Also applies to: 32-32, 52-59, 68-68, 70-70
117-141: LGTM! Improved endpoint handling with fallback logic.The code now properly:
- Extracts event engine endpoints from channel args when available
- Falls back to address resolution and TCP handshaker parameters when needed
- Handles endpoint extraction gracefully
172-237: LGTM! Clean conditional transport creation architecture.The implementation properly branches between legacy and promise-based transports based on the
GRPC_ARG_USE_V3_STACKflag. Both paths correctly initialize reading and timeout timers.
303-323: LGTM! Proper V3 stack configuration in channel creation.The
CreateHttp2Channelfunction correctly sets theGRPC_ARG_USE_V3_STACKargument based on compile-time and runtime conditions, ensuring proper transport selection.deps/grpc/src/core/ext/transport/chaotic_good/server/chaotic_good_server.cc (7)
45-46: New includes suggest external FD support expansion.The addition of
supports_fd.h,posix.h, andshim.hincludes indicates enhanced external file descriptor handling capabilities, which aligns with the newBindExternalfunctionality.Also applies to: 49-49
83-104: Well-designed unified logging functions.The new
LogInitFailureandLogInformationalhelper functions provide consistent error reporting with both standard logging and channelz integration. This centralizes logging logic and ensures proper observability.
132-201: Good abstraction with CreateListener method.The
CreateListenermethod properly abstracts listener creation logic and handles both standard and POSIX FD-based listeners. The validation logic for external FD support is appropriate.
287-290: Enhanced ActiveConnection constructor with external FD support.The constructor now properly handles external connection flags and pending data, which is essential for the new external FD binding functionality.
299-301: Proper resource cleanup in destructor.The destructor correctly handles cleanup of pending data by checking for null and calling
grpc_byte_buffer_destroy. This prevents memory leaks.
417-422: Good channelz integration in handshake setup.The handshake manager now properly receives the server's channelz base node, enabling better observability during handshakes.
646-654: Proper external address handling.The
AddChaoticGoodPortfunction correctly detects and handles "external:" prefixed addresses by callingBindExternal, which is the appropriate routing logic.deps/grpc/src/core/ext/transport/chttp2/transport/header_assembler.h (2)
44-77: Comprehensive constants and documentation.The well-defined constants and detailed comments provide clear guidance on HTTP/2 frame handling constraints and error conditions. The documentation properly references RFC9113 specifications.
225-294: Well-designed HeaderDisassembler implementation.The
HeaderDisassemblerclass provides a clean abstraction for encoding metadata into HTTP/2 frames with proper state management and frame size constraints. The implementation correctly handles both HEADERS and CONTINUATION frames.deps/grpc/src/core/client_channel/retry_interceptor.cc (6)
38-55: Clean refactoring to RetryThrottler abstraction.The
RetryStateconstructor properly accepts the newRetryThrottlerinstead of the deprecatedServerRetryThrottleData. The parameter types and member initialization are correctly updated.
69-71: Consistent usage of new throttler interface.The calls to
RecordSuccess()andRecordFailure()have been properly updated to use the newRetryThrottlerinterface, maintaining the same retry logic while using the modernized abstraction.Also applies to: 91-91
137-141: Simplified factory method using blackboard state.The new
Createmethod cleanly retrieves theRetryThrottlerfrom the blackboard state instead of parsing channel args directly. This centralizes throttler management and improves modularity.
143-160: Well-designed blackboard update mechanism.The
UpdateBlackboardmethod properly preserves existing throttler state while creating new instances based on service config changes. This ensures continuity of retry throttling across filter stack updates.
162-168: Updated constructor signature matches refactoring.The constructor properly accepts the
RetryThrottlerpointer and stores it as a member variable, completing the transition from the old throttle data approach.
194-195: Consistent initialization in Call constructor.The
Callconstructor correctly passes the interceptor'sretry_throttler_to theRetryState, maintaining proper object relationships in the refactored design.deps/grpc/include/grpc/event_engine/event_engine.h (7)
24-31: Appropriate header additions for new abstractions.The new includes for
<cstddef>,<memory>,<optional>,<utility>, andabsl/types/span.hsupport the enhanced telemetry and metrics abstractions being introduced.Also applies to: 36-36
257-263: Well-designed MetricsSet abstraction.The abstract
MetricsSetclass provides a clean interface for querying metric key membership with a singleIsSet(size_t key)method. The virtual destructor ensures proper cleanup in derived classes.
271-271: Improved ownership semantics in WriteEventSink.The change from
absl::Span<const size_t>tostd::shared_ptr<MetricsSet>clarifies ownership and lifetime management of metrics sets. The constructor and accessor properly handle the shared pointer.Also applies to: 274-274, 281-283
310-331: Proper move semantics implementation for WriteArgs.The move constructor and assignment operator are correctly implemented with proper resource transfer and nullification of moved-from state. The destructor addition suggests proper resource management.
354-356: Good API design for deprecated pointer transfer.The
TakeDeprecatedAndDiscouragedGoogleSpecificPointer()method usesstd::exchangeto properly transfer ownership while nullifying the source, following good move semantics practices.
378-400: Well-structured TelemetryInfo abstraction.The new
TelemetryInfoclass provides a comprehensive interface for querying write metrics with methods for getting all metrics, metric names/keys, and creating metric sets. This modularizes telemetry handling effectively.
429-429: Clean interface simplification.Replacing multiple metric-related methods with a single
GetTelemetryInfo()method that returns a shared pointer simplifies the Endpoint interface while maintaining functionality through the telemetry abstraction.deps/grpc/src/core/channelz/channel_trace.cc (2)
25-41: Good modernization of includes for the new architecture.The addition of
<atomic>, protobuf upb headers, and upb utilities aligns well with the new hierarchical trace system and protobuf serialization support.
243-281: Protobuf Any Type URL Consistency Verified
The Any wrapper is correctly using the standardtype.googleapis.com/grpc.channelz.v2.TraceEventprefix and matches the full message name in the upb-generated schema. Child trace serialization and type_url handling are correct—no changes needed.deps/grpc/src/core/ext/transport/chaotic_good/tcp_ztrace_collector.h (4)
80-91: LGTM: Clean addition of endpoint_id field.The addition of
endpoint_idfield toEndpointWriteMetricsTraceimproves traceability by linking metrics to specific endpoints.
94-133: Well-structured scheduling trace data.The new
TraceWriteSchedulestruct provides comprehensive scheduling metrics with proper JSON serialization. The memory usage calculation correctly accounts for the dynamicchannelsvector.
215-222: TcpZTraceCollector instantiation verified
I ran a full-text search for the removed trace types and found no lingering references toNoEndpointForWriteTraceorLbDecision. Everything in the template parameter list is up to date.
135-147: No changes needed for WriteLargeFrameHeaderTrace usage
- In deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc, the return statement
WriteLargeFrameHeaderTrace{message->payload_tag, WriteSizeForFrame(*message), *selected_reader}
correctly populates bothpayload_sizeandchosen_endpoint.- The JSON metadata type
"WRITE_LARGE_HEADER"only appears in the correspondingRenderJsonmethod and is used consistently across the codebase.All new fields are handled and the metadata string matches its sole usage.
deps/grpc/src/core/channelz/channel_trace.h (6)
54-95: Well-designed Renderer abstraction with efficient storage.The
Rendererinterface withRendererFromConcatenationprovides clean deferred rendering. TheAdaptForStoragetemplate function efficiently convertsconst char*tostring_viewto avoid unnecessary string copies.
115-143: Complex but well-designed LogExpr template.The
LogExprtemplate provides streaming syntax for trace logging with proper RAII cleanup. The move semantics and tuple manipulation are correctly implemented for efficient argument forwarding.
198-268: Excellent RAII design for Node class.The
Nodeclass provides clear ownership semantics with automatic cleanup of uncommitted entries. The move-only design prevents accidental copies and the commit mechanism allows for transactional trace entries.
378-406: TraceNode integration looks solid.The
TraceNodeclass cleanly integrates channelz tracing withGRPC_TRACElogging, with proper conditional logging based on trace flags.
457-461: Clever macro design for statement-level logging.The
GRPC_CHANNELZ_LOGmacro uses aforloop instead ofifto prevent dangling-else issues while providing streaming syntax. The detailed comment explains the security consideration well.
305-348: Entry struct size is already exposed for testing.A
testing::GetSizeofTraceEvent()function is declared indeps/grpc/src/core/channelz/channel_trace.hand defined indeps/grpc/src/core/channelz/channel_trace.ccto returnsizeof(ChannelTrace::Entry), allowing tests to detect size regressions. No further changes needed.deps/grpc/src/core/channelz/property_list.h (5)
36-46: Well-designed PropertyValue variant type.The variant covers all necessary property types including polymorphic values through
OtherPropertyValue. The inclusion of both signed/unsigned integers and the extensibleOtherPropertyValuebase class provides good flexibility.
50-112: Excellent type safety with Wrapper templates.The template specializations correctly handle type conversions:
- Integral types are normalized to
int64_t/uint64_t- Optional types are properly handled with nullopt checks
- Null pointer checks for
const char*- Proper shared_ptr wrapping for polymorphic types
121-143: PropertyList API is clean and extensible.The fluent interface with
Set()method chaining and the integration with both JSON and protobuf serialization provides a good balance of usability and extensibility.
148-170: PropertyGrid design handles ordering correctly.The separate tracking of columns and rows vectors with insertion order preservation is important for consistent output formatting.
174-198: PropertyTable with numbered rows is practical.The
AppendRow()convenience method and automatic row counting make this suitable for tabular data presentation.deps/grpc/src/core/channelz/channelz.h (5)
149-158: Good bidirectional entity type mapping.The
KindToEntityTypemethod provides clean reverse mapping from string kinds back to enum values, supporting the new channelz v2 API requirements.
288-355: Well-architected DataSink system.The new
DataSinkImplementationwith weak pointer access inDataSinkprovides good resource management. The templatedAddDatamethod with SFINAE detection ofTakeJsonObject()andFillAny()methods is elegant.
376-384: Explicit DataSource lifecycle is an improvement.The change from automatic constructor registration to explicit
SourceConstructed()/SourceDestructing()calls provides clearer lifecycle management and avoids potential race conditions.
546-549: Weak references prevent circular dependencies.The change from
RefCountedPtrtoWeakRefCountedPtrfor child socket and server socket references correctly prevents strong reference cycles that could cause memory leaks.Also applies to: 595-597
401-401: Good addition of ToPropertyList methods.The new
ToPropertyList()methods onCallCountsandSocketNode::Securitystructs integrate well with the new property-based data reporting system.Also applies to: 628-628, 636-636
deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc (7)
498-515: Excellent frame validation implementation.The frame validation logic properly enforces HTTP/2 protocol requirements, particularly for CONTINUATION frame sequences. The error handling correctly identifies protocol violations and returns appropriate connection errors.
732-774: Well-implemented stream cleanup with proper concurrency control.The CloseStream function correctly implements idempotent behavior with appropriate mutex protection and handles stream state transitions properly. The TODO for RST_STREAM frame sending is noted but doesn't affect correctness.
784-843: Robust transport shutdown implementation.The MaybeSpawnCloseTransport function properly handles concurrent access with ReleasableMutexLock, cleans up resources systematically, and uses the party system appropriately for async cleanup operations.
912-982: Well-structured outbound call processing.The CallOutboundLoop function properly implements the outbound message flow with appropriate error handling, stream validation, and promise-based concurrency. The TODO about message framing is noted for future enhancement but doesn't affect current correctness.
580-637: Solid write loop implementation with proper HTTP/2 protocol handling.The WriteLoop and WriteFromQueue functions correctly handle the client connection preface, frame serialization, and ping management integration. The promise-based approach provides good asynchronous I/O handling.
655-730: Comprehensive transport initialization.The constructor properly initializes all transport components including parties, keepalive management, ping management, and settings. The initialization sequence is logical and handles channel arguments appropriately.
33-41: Appropriate header inclusions for enhanced HTTP/2 functionality.The new includes properly support the expanded functionality including message handling, metadata processing, header assembly, and transport common utilities.
| grpc_cc_library( | ||
| name = "ssl_key_logging", | ||
| srcs = [ | ||
| "//src/core:tsi/ssl/key_logging/ssl_key_logging.cc", | ||
| ], | ||
| hdrs = [ | ||
| "//src/core:tsi/ssl/key_logging/ssl_key_logging.h", | ||
| ], | ||
| external_deps = [ | ||
| "absl/base:core_headers", | ||
| "absl/log:check", | ||
| "absl/log:log", | ||
| "absl/status", | ||
| "absl/status:statusor", | ||
| "absl/strings", | ||
| "libcrypto", | ||
| "libssl", | ||
| ], | ||
| visibility = ["//visibility:public"], | ||
| deps = [ | ||
| "channel_args", | ||
| "env", | ||
| "error", | ||
| "grpc_transport_chttp2_alpn", | ||
| "load_file", | ||
| "ref_counted", | ||
| "slice", | ||
| "sync", | ||
| "tsi_ssl_types", | ||
| "useful", | ||
| "//:channel_arg_names", | ||
| "//:config_vars", | ||
| "//:gpr", | ||
| "//:grpc_base", | ||
| "//:grpc_core_credentials_header", | ||
| "//:grpc_credentials_util", | ||
| "//:grpc_public_hdrs", | ||
| "//:grpc_security_base", | ||
| "//:ref_counted_ptr", | ||
| "//:transport_auth_context", | ||
| "//:tsi_base", | ||
| "//:tsi_ssl_session_cache", | ||
| ], |
There was a problem hiding this comment.
Public export of TLS key-logging utility is risky
ssl_key_logging is given //visibility:public, enabling accidental linkage of code that writes TLS session keys to disk. Consider keeping it private and gating its inclusion behind a build flag to avoid unintentional credential leakage in production binaries.
🤖 Prompt for AI Agents
In deps/grpc/src/core/BUILD around lines 10262 to 10304, the grpc_cc_library
target "ssl_key_logging" is marked with public visibility, which risks
accidental linkage of TLS key-logging code in production. Change the visibility
from public to private and add a build flag condition to include this target
only when explicitly enabled, preventing unintentional inclusion of sensitive
key-logging functionality in production builds.
| BaseNode(EntityType type, size_t max_trace_memory, std::string name); | ||
|
|
||
| // Leaf derived types should call NodeConstructed() in their constructors | ||
| // to complete the initialization. | ||
| void NodeConstructed(); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify NodeConstructed() lifecycle management.
The requirement for leaf classes to call NodeConstructed() in their constructors adds a lifecycle step. Ensure all derived classes properly implement this pattern to avoid registration issues.
🏁 Script executed:
#!/bin/bash
# Verify all Node classes call NodeConstructed()
rg -A 5 "NodeConstructed"
ast-grep --pattern 'class $_ : public BaseNode {
$$$
$_($$$) : BaseNode($$$) {
$$$
}
$$$
}'Length of output: 3295
Enforce immediate registration by calling NodeConstructed() in every derived-class constructor
ChannelNode already invokes NodeConstructed() inline in its header constructor. However, the other leaf types defer registration until their first "use" method, which risks missing or late registrations. Update each of the following to call NodeConstructed() directly in their constructors (immediately after BaseNode initialization):
- SubchannelNode (currently in UpdateConnectivityState in channelz.cc)
- ServerNode (currently in RenderServerSockets in channelz.cc)
- SocketNode (currently in RecordStreamStartedFromLocal in channelz.cc)
- ListenSocketNode (currently in RenderJson in channelz.cc)
For example, in channelz.cc:
class SubchannelNode : public BaseNode {
+ public:
+ SubchannelNode(/* params */)
+ : BaseNode(EntityType::SUBCHANNEL, /* max_trace_memory */, /* name */) {
+ NodeConstructed();
+ }
…
};Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In deps/grpc/src/core/channelz/channelz.h and corresponding channelz.cc around
lines 161 to 165, the leaf node classes SubchannelNode, ServerNode, SocketNode,
and ListenSocketNode delay calling NodeConstructed() until their first use,
risking late registration. To fix this, modify each of these classes'
constructors to call NodeConstructed() immediately after the BaseNode
constructor call, ensuring registration happens as soon as the object is created
rather than deferred.
| scheduling_state_.store(kSchedulingProcessing, | ||
| std::memory_order_relaxed); |
There was a problem hiding this comment.
Use release ordering for scheduling_state_ store
This store uses memory_order_relaxed but the destructor in data_endpoints.h loads with memory_order_acquire. Without release ordering here, there's no happens-before relationship.
- scheduling_state_.store(kSchedulingProcessing,
- std::memory_order_relaxed);
+ scheduling_state_.store(kSchedulingProcessing,
+ std::memory_order_release);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| scheduling_state_.store(kSchedulingProcessing, | |
| std::memory_order_relaxed); | |
| scheduling_state_.store(kSchedulingProcessing, | |
| std::memory_order_release); |
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.cc at lines
297-298, the store to scheduling_state_ uses memory_order_relaxed but the
corresponding load in the destructor uses memory_order_acquire. To establish a
proper happens-before relationship, change the store's memory order to
memory_order_release instead of memory_order_relaxed.
| uint64_t WriteSizeForFrame(const QueuedFrame& queued_frame) { | ||
| auto& frame = | ||
| absl::ConvertVariantTo<FrameInterface&>(queued_frame.frame->payload); | ||
| const auto hdr = frame.MakeHeader(); | ||
| const size_t length = hdr.payload_length; | ||
| return TcpDataFrameHeader::kFrameHeaderSize + | ||
| DataConnectionPadding(TcpDataFrameHeader::kFrameHeaderSize, | ||
| encode_alignment_) + | ||
| length + DataConnectionPadding(length, encode_alignment_); | ||
| } |
There was a problem hiding this comment.
Validate frame variant before accessing FrameInterface
The WriteSizeForFrame method uses absl::ConvertVariantTo<FrameInterface&> without checking if the conversion is valid. If the variant doesn't contain a FrameInterface, this could lead to undefined behavior.
Consider using absl::get_if to safely check the variant type before accessing:
uint64_t WriteSizeForFrame(const QueuedFrame& queued_frame) {
- auto& frame =
- absl::ConvertVariantTo<FrameInterface&>(queued_frame.frame->payload);
+ auto* frame = absl::get_if<FrameInterface>(&queued_frame.frame->payload);
+ if (frame == nullptr) {
+ LOG(ERROR) << "Invalid frame type in queued_frame";
+ return 0;
+ }
- const auto hdr = frame.MakeHeader();
+ const auto hdr = frame->MakeHeader();
const size_t length = hdr.payload_length;
return TcpDataFrameHeader::kFrameHeaderSize +
DataConnectionPadding(TcpDataFrameHeader::kFrameHeaderSize,
encode_alignment_) +
length + DataConnectionPadding(length, encode_alignment_);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint64_t WriteSizeForFrame(const QueuedFrame& queued_frame) { | |
| auto& frame = | |
| absl::ConvertVariantTo<FrameInterface&>(queued_frame.frame->payload); | |
| const auto hdr = frame.MakeHeader(); | |
| const size_t length = hdr.payload_length; | |
| return TcpDataFrameHeader::kFrameHeaderSize + | |
| DataConnectionPadding(TcpDataFrameHeader::kFrameHeaderSize, | |
| encode_alignment_) + | |
| length + DataConnectionPadding(length, encode_alignment_); | |
| } | |
| uint64_t WriteSizeForFrame(const QueuedFrame& queued_frame) { | |
| auto* frame = absl::get_if<FrameInterface>(&queued_frame.frame->payload); | |
| if (frame == nullptr) { | |
| LOG(ERROR) << "Invalid frame type in queued_frame"; | |
| return 0; | |
| } | |
| const auto hdr = frame->MakeHeader(); | |
| const size_t length = hdr.payload_length; | |
| return TcpDataFrameHeader::kFrameHeaderSize + | |
| DataConnectionPadding(TcpDataFrameHeader::kFrameHeaderSize, | |
| encode_alignment_) + | |
| length + DataConnectionPadding(length, encode_alignment_); | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chaotic_good/data_endpoints.h around lines
237 to 246, the method WriteSizeForFrame uses absl::ConvertVariantTo to cast the
queued_frame's payload to FrameInterface& without verifying the variant's actual
type, risking undefined behavior. Modify the code to use absl::get_if to safely
check if the variant holds a FrameInterface before accessing it. If the check
fails, handle the error appropriately, such as returning a default size or
logging an error, to prevent unsafe access.
| if (stream == nullptr) { | ||
| // TODO(tjagtap) : [PH2][P2] : Implement the correct behaviour later. | ||
| // RFC9113 : If a DATA frame is received whose stream is not in the "open" | ||
| // or "half-closed (local)" state, the recipient MUST respond with a stream | ||
| // error (Section 5.4.2) of type STREAM_CLOSED. | ||
| GRPC_HTTP2_CLIENT_DLOG | ||
| << "Http2Transport ProcessHttp2DataFrame { stream_id=" | ||
| << frame.stream_id << "} Lookup Failed"; | ||
| return Http2Status::Ok(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper error handling for DATA frames on unknown streams.
According to RFC 9113, receiving a DATA frame for a stream not in the "open" or "half-closed (local)" state should result in a stream error of type STREAM_CLOSED, not a silent success.
Consider implementing the proper response:
if (stream == nullptr) {
- // TODO(tjagtap) : [PH2][P2] : Implement the correct behaviour later.
- // RFC9113 : If a DATA frame is received whose stream is not in the "open"
- // or "half-closed (local)" state, the recipient MUST respond with a stream
- // error (Section 5.4.2) of type STREAM_CLOSED.
- GRPC_HTTP2_CLIENT_DLOG
- << "Http2Transport ProcessHttp2DataFrame { stream_id="
- << frame.stream_id << "} Lookup Failed";
- return Http2Status::Ok();
+ GRPC_HTTP2_CLIENT_DLOG
+ << "Http2Transport ProcessHttp2DataFrame { stream_id="
+ << frame.stream_id << "} Lookup Failed - sending STREAM_CLOSED error";
+ return Http2Status::Http2StreamError(
+ Http2ErrorCode::kStreamClosed,
+ absl::StrCat("Received DATA frame for unknown stream ", frame.stream_id));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (stream == nullptr) { | |
| // TODO(tjagtap) : [PH2][P2] : Implement the correct behaviour later. | |
| // RFC9113 : If a DATA frame is received whose stream is not in the "open" | |
| // or "half-closed (local)" state, the recipient MUST respond with a stream | |
| // error (Section 5.4.2) of type STREAM_CLOSED. | |
| GRPC_HTTP2_CLIENT_DLOG | |
| << "Http2Transport ProcessHttp2DataFrame { stream_id=" | |
| << frame.stream_id << "} Lookup Failed"; | |
| return Http2Status::Ok(); | |
| } | |
| if (stream == nullptr) { | |
| GRPC_HTTP2_CLIENT_DLOG | |
| << "Http2Transport ProcessHttp2DataFrame { stream_id=" | |
| << frame.stream_id << "} Lookup Failed - sending STREAM_CLOSED error"; | |
| return Http2Status::Http2StreamError( | |
| Http2ErrorCode::kStreamClosed, | |
| absl::StrCat("Received DATA frame for unknown stream ", frame.stream_id)); | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
around lines 147 to 156, the code currently logs and silently returns Ok() when
a DATA frame is received for a stream that is nullptr, which violates RFC 9113.
Update the code to respond with a stream error of type STREAM_CLOSED instead of
returning Ok(). Implement the logic to send this stream error back to the sender
as per the HTTP/2 specification.
| if (stream == nullptr) { | ||
| // TODO(tjagtap) : [PH2][P3] : Implement this. | ||
| // RFC9113 : The identifier of a newly established stream MUST be | ||
| // numerically greater than all streams that the initiating endpoint has | ||
| // opened or reserved. This governs streams that are opened using a HEADERS | ||
| // frame and streams that are reserved using PUSH_PROMISE. An endpoint that | ||
| // receives an unexpected stream identifier MUST respond with a connection | ||
| // error (Section 5.4.1) of type PROTOCOL_ERROR. | ||
| return Http2Status::Ok(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper error handling for CONTINUATION frames on unknown streams.
Similar to HEADERS frames, receiving a CONTINUATION frame for an unknown stream should result in a connection error of type PROTOCOL_ERROR per RFC 9113, not a silent success.
if (stream == nullptr) {
- // TODO(tjagtap) : [PH2][P3] : Implement this.
- // RFC9113 : The identifier of a newly established stream MUST be
- // numerically greater than all streams that the initiating endpoint has
- // opened or reserved. This governs streams that are opened using a HEADERS
- // frame and streams that are reserved using PUSH_PROMISE. An endpoint that
- // receives an unexpected stream identifier MUST respond with a connection
- // error (Section 5.4.1) of type PROTOCOL_ERROR.
- return Http2Status::Ok();
+ return Http2Status::Http2ConnectionError(
+ Http2ErrorCode::kProtocolError,
+ absl::StrCat("Received CONTINUATION frame for unknown stream ", frame.stream_id));
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (stream == nullptr) { | |
| // TODO(tjagtap) : [PH2][P3] : Implement this. | |
| // RFC9113 : The identifier of a newly established stream MUST be | |
| // numerically greater than all streams that the initiating endpoint has | |
| // opened or reserved. This governs streams that are opened using a HEADERS | |
| // frame and streams that are reserved using PUSH_PROMISE. An endpoint that | |
| // receives an unexpected stream identifier MUST respond with a connection | |
| // error (Section 5.4.1) of type PROTOCOL_ERROR. | |
| return Http2Status::Ok(); | |
| } | |
| if (stream == nullptr) { | |
| return Http2Status::Http2ConnectionError( | |
| Http2ErrorCode::kProtocolError, | |
| absl::StrCat("Received CONTINUATION frame for unknown stream ", frame.stream_id)); | |
| } |
🤖 Prompt for AI Agents
In deps/grpc/src/core/ext/transport/chttp2/transport/http2_client_transport.cc
around lines 404 to 413, the code currently returns a silent success when a
CONTINUATION frame is received for an unknown stream. To fix this, replace the
silent return with proper error handling that triggers a connection error of
type PROTOCOL_ERROR as specified by RFC 9113. Implement logic to detect the
unknown stream and respond with the appropriate connection error instead of
returning Http2Status::Ok().
santigimeno
force-pushed
the
santi/grpc_1.74.0
branch
from
84b2296 to
843ccb8
Compare
PR-URL: #350 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
|
Landed in 240a1cf |
PR-URL: #350 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
2 participants
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Chores