Remove dead config records from NetHandler - #13533
Conversation
Three proxy.config.net.* records have been unregistered since their introduction in 2015 (TS-3313), so their callbacks never fired and the struct fields they wrote were never read. Removing them shifts default_inactivity_timeout to index 2; the bitset selecting per-thread dependent values is unaffected since those members keep indices 0 and 1. Assertions now pin the field offsets and the bitset width, both of which were previously implicit. Fixes: apache#12933
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Removes long-dead proxy.config.net.*_in configuration records from NetHandler and hardens assumptions about NetHandler::Config layout / per-thread-dependent config selection.
Changes:
- Removed reads, update callbacks, and debug logging for three unregistered config records.
- Updated
NetHandler::Configto drop the unused fields and shiftdefault_inactivity_timeoutto index 2. - Added compile-time assertions for
Configmember offsets and for the per-thread-dependent mask used to initialize astd::bitset.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/iocore/net/UnixNet.cc | Centralizes the per-thread-dependent config mask and adds a compile-time guard before initializing the bitset. |
| src/iocore/net/NetHandler.cc | Removes dead config reads / Rec callbacks / debug lines for unregistered records. |
| include/iocore/net/NetHandler.h | Removes unused config fields and adds offsetof-based layout assertions for the array-like indexing contract. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address review feedback. Assert the standard layout and alignment that offsetof and the array-like operator[] rely on, and replace the mask fit check with std::bit_width so it does not depend on a shift that would be ill formed if the field count ever reached the width of the shifted type.
The Ubuntu CI toolchain (clang 12) does not provide std::bit_width, so the mask-width check broke the build there. Assert the mask fits using a shift instead, guarded by a bit-width check so the shift itself stays well defined.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
include/iocore/net/NetHandler.h:139
- These assertions do not make
Configsafely indexable as an array.operator[]advances a pointer to one data member, and the update callback subtracts pointers to distinct members; both operations are undefined even whenoffsetofproves the members contiguous. Also,CONFIG_ITEM_COUNTcan include tail padding becausesizeof(Config)is not constrained here. Please represent the indexed values withstd::array<uint32_t, 3>(or use explicit member-to-index mapping) so indexing and the bitset width are defined.
// Config is addressed as an array of uint32_t through operator[], and
// config_value_affects_per_thread_value is a bitset indexed by field
// position, so the offset of each member is part of the interface.
static_assert(std::is_standard_layout_v<Config>); // required for offsetof below to be well defined
static_assert(alignof(Config) == alignof(uint32_t)); // a member of wider type would break operator[]
static_assert(offsetof(Config, max_connections_in) == 0 * sizeof(uint32_t));
static_assert(offsetof(Config, max_requests_in) == 1 * sizeof(uint32_t));
static_assert(offsetof(Config, default_inactivity_timeout) == 2 * sizeof(uint32_t));
JosiahWI
left a comment
JosiahWI
left a comment
There was a problem hiding this comment.
Copilot is right that array-indexing into the config structure is UB, but that is out of scope here. Looks good.
yes, there will be a follow up for that. Didn't want to mix it in here(although it may remove some of this code). thanks. |
Three proxy.config.net.* records have been unregistered since their introduction in 2015 , so their callbacks never fired and the struct fields they wrote were never read. Removing them shifts default_inactivity_timeout to index 2; the bitset selecting per-thread dependent values is unaffected since those members keep indices 0 and 1. Assertions now pin the field offsets and the bitset width, both of which were previously implicit.
Fixes: #12933