Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: pass requests by value #15971

Merged
merged 1 commit into from
Jan 6, 2024
Merged

Conversation

rockwotj
Copy link
Contributor

@rockwotj rockwotj commented Jan 5, 2024

Pass RPC requests by value, as we start to write some handlers using
coroutines, it can be a footgun to access have a suspension point, then
the request is destroyed. An example that motivated this:

ss::future<reply> service::handle(request&& r, rpc::streaming_context&) {
  co_await ss::coroutine::switch_to(get_scheduling_group());
  co_return co_await handle_request(std::move(r));
}

In this case r is destroyed because the coroutine does not own the
value, and handle request would have a stack-use-after-return issue.

Now that we pass by value, we don't have to remember to move the value
into the current method.

Fixes: https://github.com/redpanda-data/core-internal/issues/898

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

  • none

Pass RPC requests by value, as we start to write some handlers using
coroutines, it can be a footgun to access have a suspension point, then
the request is destroyed. An example that motivated this:

```c++
ss::future<reply> service::handle(request&& r, rpc::streaming_context&) {
  co_await ss::coroutine::switch_to(get_scheduling_group());
  co_return co_await handle_request(std::move(r));
}
```

In this case `r` is destroyed because the coroutine does not own the
value, and handle request would have a stack-use-after-return issue.

Now that we pass by value, we don't have to remember to move the value
into the current method.

Signed-off-by: Tyler Rockwood <rockwood@redpanda.com>
@vbotbuildovich
Copy link
Collaborator

new failures in https://buildkite.com/redpanda/redpanda/builds/43498#018cdb8f-4871-4c46-9031-75627717886a:

"rptest.tests.recovery_mode_test.DisablingPartitionsTest.test_disable"
"rptest.tests.cluster_quota_test.ClusterRateQuotaTest.test_client_group_produce_rate_throttle_mechanism"

Copy link
Contributor

@bharathv bharathv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

@rockwotj
Copy link
Contributor Author

rockwotj commented Jan 6, 2024

CI Failure: #15972

@vbotbuildovich
Copy link
Collaborator

@rockwotj rockwotj merged commit c67ce52 into redpanda-data:dev Jan 6, 2024
22 checks passed
@rockwotj rockwotj deleted the rpc-followup branch January 6, 2024 12:50
@emaxerrno
Copy link
Contributor

I wonder if we should enforce all types in RPC via a concept to be either small pods of say 16bytes or non copyable and non copy assignment.

@rockwotj
Copy link
Contributor Author

rockwotj commented Jan 7, 2024

Yeah not against making stuff explicitly copyable only, or doing this via serde where you have to add a flag to opt into copy constructor if not trivially copyable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants