kafka/server: remove foreign_ptr from response_ptr#30042
kafka/server: remove foreign_ptr from response_ptr#30042travisdowns merged 2 commits intoredpanda-data:devfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR simplifies Kafka response lifetime management by removing the ss::foreign_ptr wrapper from response_ptr, based on the invariant that response objects are allocated and destroyed on the connection’s shard. It adds a debug-only cross-shard destruction check via a new oncore_auto helper embedded in kafka::response.
Changes:
- Introduce
oncore_auto(destructor verifies same-shard in debug builds) inbase/oncore.h. - Embed
oncore_autointokafka::responseand changeresponse_ptrtostd::unique_ptr<response>. - Update related includes/commentary to reflect the new ownership model (e.g. scattered message lifetime note).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/v/kafka/server/response.h | Embed oncore_auto in response and switch response_ptr to std::unique_ptr. |
| src/v/kafka/server/protocol_utils.cc | Clarify that the response must outlive scattered_message due to fragment references. |
| src/v/kafka/server/connection_context.h | Remove local response_ptr alias and include response.h for the canonical definition. |
| src/v/base/oncore.h | Add oncore_auto helper to assert same-shard destruction in debug builds. |
CI test resultstest results on build#82648
test results on build#82682
|
pgellert
left a comment
pgellert
left a comment
There was a problem hiding this comment.
lgtm, just one question about the no_unique_address
| // NOLINTNEXTLINE(cppcoreguidelines-special-member-functions,hicpp-special-member-functions) | ||
| struct oncore_auto final { | ||
| ~oncore_auto() noexcept { oncore_debug_verify(_oncore); } | ||
| [[no_unique_address]] oncore _oncore; |
There was a problem hiding this comment.
For [[no_unique_address]] to work, are we relying here on the compiler optimizing away the unused oncore::_owner_shard field in release builds? Is that a safe assumption, or would we need to wrap the field in an ifndef NDEBUG macro as well?
There was a problem hiding this comment.
Compiler will ~never optimize away unused fields in an object. So this annotation has no purpose here, as the object it is applied to is never empty. So I will remove it.
I think a better overall pattern for objects like this where you sometimes want to do it in debug, or sometimes in release, is to have the object itself (oncore) do the hard work, and have two variants oncore and oncore_debug, where the former is "always on" and the latter is on only in DEBUG but, both offer the same API regardless of compile mode (but "debug" version function is empty and object is empty as well in release mode), then the consumers can use them with little friction: no conditional compilation, only need [[no_unique_address]] on members.
There was a problem hiding this comment.
Removed it, it wasn't doing anything useful here since oncore is never empty.
An oncore that debug-asserts same-shard in its destructor. Embed as a member to catch cross-shard destruction in debug builds.
travisdowns
dismissed stale reviews from pgellert and StephanDollberg
via
64a6b6a
travisdowns
force-pushed
the
td-remove-response-foreign-ptr
branch
from
71c8567 to
64a6b6a
Compare
| std::optional<tagged_fields> _tags; | ||
| iobuf _buf; | ||
| protocol::encoder _writer; | ||
| [[no_unique_address]] oncore_auto _oncore; |
There was a problem hiding this comment.
nit: there's one more of this
| [[no_unique_address]] oncore_auto _oncore; | |
| oncore_auto _oncore; |
There was a problem hiding this comment.
Oops, sorry about that — removed. Should have caught both in the first pass.
The response object is always created and destroyed on the connection's shard. The foreign_ptr wrapper was a historical artifact or oversight, since in general a response may contain data from many shards so a single-response level foreign pointer doesn't work. Today cross shard deletion is handled by things _inside_ the response, e.g., by foreign_record_batch_readers nested somewhere inside the response. The respond() path in request_context always serializes into fresh local buffers, so the response iobuf never contains foreign fragments. Replace foreign_ptr with a plain unique_ptr and add an oncore_auto member to response to assert same-shard destruction in debug builds.
travisdowns
force-pushed
the
td-remove-response-foreign-ptr
branch
from
64a6b6a to
f3a9454
Compare
The
response_ptrtype wasss::foreign_ptr<std::unique_ptr<response>>,but the
foreign_ptrwrapper is unnecessary: the response is alwaysallocated and destroyed on the connection's shard. The wrapper was added
in 2020 (1313f09) when
iobufstopped forwarding fragment deletersto the home core, but this is now handled at a higher level by
foreign_record_batch_reader. Therespond()path inrequest_contextserializes into fresh local buffers, so the response iobuf never
contains foreign fragments.
This PR adds an
oncore_autohelper (anoncorethat debug-assertssame-shard in its destructor), embeds it in
response, and replacesforeign_ptr<unique_ptr<response>>with a plainunique_ptr<response>.Backports Required
Release Notes