Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/rpc/repe-buffer.md
Original file line number Diff line number Diff line change
Expand Up @@ -261,14 +261,14 @@ template <auto Opts, class Value>
bool read_params(Value&& value, state_view& state);
```

Returns `true` on success. On a parse failure it writes the error response through `state.out` itself, so a handler should return without writing anything further.
Returns `true` on success. On failure it writes the error response through `state.out` itself, so a handler should return without writing anything further. An empty body is a failure like any other and is answered with `no_read_input`. The exception is a notification, which is answered by silence whether the read succeeds or fails: `read_params` returns `false` having left `state.out` untouched. Returning immediately on `false` is correct in both cases, but a handler that inspects the response buffer afterwards has to allow for it being empty.

`Opts` must have `null_terminated` turned off. A request is a span over bytes the handler does not own with no `'\0'` after it, and a `null_terminated` read drops its end checks and runs past the buffer. Use `glz::registry_read_opts<Opts>`, which is the transform the registry applies to its own options:

```cpp
if (state.has_body()) {
if (!glz::repe::read_params<glz::registry_read_opts<glz::opts{}>>(params, state)) {
return; // the error response is already written
return; // the error response is already written, or withheld from a notification
}
}
```
Expand Down
8 changes: 5 additions & 3 deletions docs/rpc/repe-rpc.md
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ The zero-copy API uses these types:
- **`glz::repe::request_view`**: Views into the original request buffer (query and body are `std::string_view`).
- **`glz::repe::response_builder`**: Writes responses directly to a buffer without intermediate copies.
- **`glz::repe::state_view`**: Pairs a `request_view` with a `response_builder` for a procedure to read from and write to.
- **`glz::repe::read_params<Opts>(value, state)`**: Reads a request body into `value`. Returns `true` on success and writes the error response itself on a parse failure.
- **`glz::repe::read_params<Opts>(value, state)`**: Reads a request body into `value`. Returns `true` on success and writes the error response itself on failure, except for a notification, which is left unanswered.

See [REPE Buffer Handling](repe-buffer.md) for detailed documentation of these types.

Expand All @@ -122,19 +122,21 @@ server.call = [&](std::span<const char> request, std::string& response_buffer) {
my_params params{};
if (state.has_body()) {
if (!glz::repe::read_params<glz::registry_read_opts<glz::opts{}>>(params, state)) {
return; // read_params has written the error response
return; // read_params has written the error response, or withheld it from a notification
}
}
// ... act on params, then write a response through `resp`
};
```

Two things are easy to get wrong:
Three things are easy to get wrong:

**`Opts` must have `null_terminated` turned off.** A request arrives as a span over bytes the handler does not own, with no `'\0'` after it, and a `null_terminated` read drops its end checks and runs off the end of that buffer. `glz::registry_read_opts<Opts>` is the registry's own options transform and turns the flag off for you; passing a bare `glz::opts{}` is a heap overflow on a body that ends at the edge of the buffer.

**`state` must be a named lvalue.** The parameter is `state_view&`. A temporary binds to a different overload and fails to compile inside the header rather than at your call site.

**`false` does not always mean a response was written.** A notification is answered by silence whether the read succeeds or fails, so a notification whose body will not parse returns `false` with `state.out` untouched. Returning immediately on `false`, as above, is correct either way. A handler that instead inspects the response buffer, or appends to it, has to allow for it being empty: answering a notification desynchronizes the connection, because the client never reads a reply for one and will take it as the answer to the next call.

#### `read_params` returns `bool`

It returned the number of bytes consumed through v7.9.1, and callers tested that count for zero to detect failure:
Expand Down
9 changes: 8 additions & 1 deletion include/glaze/rpc/registry.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -460,8 +460,15 @@ namespace glz
return;
}

// If request has an error, just echo it back
// If request has an error, just echo it back -- unless it is a notification, whose sender
// has said it will not read a reply, so answering one desynchronizes the connection: the
// client takes the echo as the answer to its next call. The header parsed and validated
// cleanly to reach here, so its notify bit can be trusted, which is what separates this
// from the malformed-header paths above.
if (bool(req.hdr.ec)) {
if (req.is_notify()) {
return; // Silent ignore for a notification that carries an error (buffer stays empty)
}
resp.reset(req);
resp.set_error(req.hdr.ec);
return;
Expand Down
66 changes: 44 additions & 22 deletions include/glaze/rpc/repe/repe.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -109,30 +109,46 @@ namespace glz::repe
}
}

// Returns false on error. The byte count the reader reports cannot stand in for success here:
// without a null terminator a variant alternative that resolves at the end of the buffer rewinds
// its iterator, so a completed read can report zero bytes consumed.
// Returns false on error, having written the error response -- unless the request is a
// notification, which is answered by silence whether the read succeeds or fails. A caller that
// returns immediately on false is correct in both cases; one that inspects state.out afterwards
// has to allow for it being untouched.
//
// The byte count the reader reports cannot stand in for success here: without a null terminator
// a variant alternative that resolves at the end of the buffer rewinds its iterator, so a
// completed read can report zero bytes consumed.
template <auto Opts, class Value>
bool read_params(Value&& value, auto&& state)
{
glz::context ctx{};
auto [b, e] = read_iterators<Opts>(state.in.body);
auto start = b;

// An empty body is a failure like any other and has to answer like one. Returning early left
// a non-notify request with no response at all: every caller is told the error response is
// written whenever this returns false, and every registered endpoint returns immediately on
// it. A parameterized function endpoint reads without a has_body() guard -- it has nothing to
// call the function with otherwise -- so an empty body reached here and the client was left
// waiting on a reply that was never sent.
if (state.in.body.empty()) [[unlikely]] {
ctx.error = error_code::no_read_input;
}
if (bool(ctx.error)) [[unlikely]] {
return false;
else {
glz::parse<Opts.format>::template op<is_padded_off<Opts>()>(std::forward<Value>(value), ctx, b, e);
// This bypasses glz::read, so the bookkeeping glz::read performs after the parse has to be
// repeated here: a value that finishes exactly at the end of the body reports end_reached,
// which is a completed read rather than an error, while a body that held no value at all is
// not. finalize_top_level_read draws that line.
finalize_top_level_read<Opts>(ctx, start, b, e);
}
auto start = b;

glz::parse<Opts.format>::template op<is_padded_off<Opts>()>(std::forward<Value>(value), ctx, b, e);
// This bypasses glz::read, so the bookkeeping glz::read performs after the parse has to be
// repeated here: a value that finishes exactly at the end of the body reports end_reached,
// which is a completed read rather than an error, while a body that held no value at all is
// not. finalize_top_level_read draws that line.
finalize_top_level_read<Opts>(ctx, start, b, e);

if (bool(ctx.error)) {
// A notification is a request the sender has said it will not read a reply to, so a failed
// read of one is reported by not answering, exactly as a successful read of one is.
if (state.notify()) {
return false;
}

state.out.header.ec = ctx.error;
error_ctx ec{size_t(b - start), ctx.error, ctx.custom_error_message};

Expand Down Expand Up @@ -544,29 +560,35 @@ namespace glz::repe
concept is_state_view = std::same_as<std::decay_t<T>, state_view>;

/// Read parameters from state_view (zero-copy from input buffer)
/// Returns false on error (error set in state.out); see the note on the overload above for why
/// this is not a byte count.
/// Returns false on error, with the error set in state.out -- except for a notification, which
/// is left unanswered and so leaves state.out untouched. See the note on the overload above for
/// why this is not a byte count.
template <auto Opts, class Value>
bool read_params(Value&& value, state_view& state)
{
glz::context ctx{};
auto body = state.in.body;
auto b = body.data();
auto e = b + body.size();
auto start = b;

// An empty body answers like any other failure; see the note in the message overload above.
if (body.empty()) [[unlikely]] {
ctx.error = error_code::no_read_input;
}
if (bool(ctx.error)) [[unlikely]] {
return false;
else {
glz::parse<Opts.format>::template op<is_padded_off<Opts>()>(std::forward<Value>(value), ctx, b, e);
// See the note in the message overload above for why glz::read's bookkeeping is repeated.
finalize_top_level_read<Opts>(ctx, start, b, e);
}
auto start = b;

glz::parse<Opts.format>::template op<is_padded_off<Opts>()>(std::forward<Value>(value), ctx, b, e);
// See the note in the message overload above for why glz::read's bookkeeping is repeated.
finalize_top_level_read<Opts>(ctx, start, b, e);

if (bool(ctx.error)) {
// A notification is a request the sender has said it will not read a reply to, so a failed
// read of one is reported by not answering, exactly as a successful read of one is.
if (state.notify()) {
return false;
}

error_ctx ec{size_t(b - start), ctx.error, ctx.custom_error_message};
std::string error_message = format_error(ec, body);
state.out.reset(state.in);
Expand Down
90 changes: 89 additions & 1 deletion tests/networking_tests/registry_view_test/registry_view_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// For the license information refer to glaze.hpp

#include <cstring>
#include <functional>
#include <string>
#include <variant>
#include <vector>
Expand All @@ -15,13 +16,15 @@ using namespace ut;
namespace
{
// Helper to create a valid REPE request buffer
std::string make_request(std::string_view query, std::string_view body, uint64_t id = 1, bool notify = false)
std::string make_request(std::string_view query, std::string_view body, uint64_t id = 1, bool notify = false,
glz::error_code ec = glz::error_code::none)
{
glz::repe::header hdr{};
hdr.spec = glz::repe::repe_magic;
hdr.version = 1;
hdr.id = id;
hdr.notify = notify ? 1 : 0;
hdr.ec = ec;
hdr.query_length = query.size();
hdr.body_length = body.size();
hdr.length = sizeof(glz::repe::header) + query.size() + body.size();
Expand Down Expand Up @@ -330,6 +333,13 @@ struct inferred_api
std::variant<std::string, amount> measure{};
};

// A std::function member registers as a call with typed parameters, which is the one endpoint kind
// that reads a body without first checking whether there is one.
struct param_fn_api
{
std::function<int(int)> doubled = [](int v) { return v * 2; };
};

suite unterminated_buffer_tests = [] {
"scalar_body_ending_at_buffer_end"_test = [] {
glz::registry<> registry;
Expand Down Expand Up @@ -411,6 +421,84 @@ suite unterminated_buffer_tests = [] {
expect(result.request.error() == glz::error_code::none) << "No error expected";
};

// A parameterized function endpoint has nothing to call its function with when the body is
// empty, so it reads without the has_body() guard the value endpoints use. read_params returned
// false for an empty body without writing anything, and the endpoint returns immediately on
// false, so the request went unanswered and its client waited on a reply that was never coming.
"empty_body_to_a_param_function_is_answered"_test = [] {
glz::registry<> registry;
param_fn_api api{};
registry.on(api);

const auto request = exact_buffer(make_request("/doubled", ""));
std::string response_buf;
registry.call(std::span<const char>{request.data(), request.size()}, response_buf);

expect(!response_buf.empty()) << "A non-notify request must always get a response";
auto result = glz::repe::parse_request({response_buf.data(), response_buf.size()});
expect(bool(result)) << "Response should be parseable";
expect(result.request.error() == glz::error_code::no_read_input) << "Expected no_read_input";
};

"a param function still answers a good body"_test = [] {
glz::registry<> registry;
param_fn_api api{};
registry.on(api);

const auto request = exact_buffer(make_request("/doubled", "21"));
std::string response_buf;
registry.call(std::span<const char>{request.data(), request.size()}, response_buf);

auto result = glz::repe::parse_request({response_buf.data(), response_buf.size()});
expect(bool(result));
expect(result.request.error() == glz::error_code::none);
expect(result.request.body == "42") << "the function should have run";
};

// The other half of the same rule: a notification is a request the sender has said it will not
// read a reply to, so a read that fails on one is reported by not answering it.
"a failed read of a notification is not answered"_test = [] {
for (std::string_view body : {"", "{", "not json"}) {
glz::registry<> registry;
param_fn_api api{};
registry.on(api);

const auto request = exact_buffer(make_request("/doubled", body, 1, true));
std::string response_buf;
registry.call(std::span<const char>{request.data(), request.size()}, response_buf);

expect(response_buf.empty()) << "a notification must not be answered: " << body;
}
};

// The registry echoes a request that already carries an error back to its sender. A notification
// has no sender waiting on that echo, so it is dropped like every other unanswerable request.
"a notification carrying an error is not echoed"_test = [] {
glz::registry<> registry;
unterminated_api api{};
registry.on(api);

const auto request = exact_buffer(make_request("/value", "", 1, true, glz::error_code::parse_error));
std::string response_buf;
registry.call(std::span<const char>{request.data(), request.size()}, response_buf);

expect(response_buf.empty()) << "a notification must not be answered";
};

"a request carrying an error is still echoed"_test = [] {
glz::registry<> registry;
unterminated_api api{};
registry.on(api);

const auto request = exact_buffer(make_request("/value", "", 1, false, glz::error_code::parse_error));
std::string response_buf;
registry.call(std::span<const char>{request.data(), request.size()}, response_buf);

auto result = glz::repe::parse_request({response_buf.data(), response_buf.size()});
expect(bool(result)) << "Response should be parseable";
expect(result.request.error() == glz::error_code::parse_error) << "the error should be echoed back";
};

// A registry that accepts comments must reject a body that holds nothing but one, for the same
// reason it rejects a body that holds nothing but whitespace.
"comment_only_body_is_an_error"_test = [] {
Expand Down
Loading