Answer a REPE request whose body is empty - #2739
Merged
Merged
Conversation
read_params reported an empty body by returning false without writing anything. Every caller is told the error response is written whenever it returns false, and every registered endpoint returns immediately on it, so nothing else wrote one either. Most endpoints never reached it: they guard on state.has_body() and skip the read. A parameterized function endpoint cannot, since it has nothing to call its function with otherwise, so it read unconditionally. A non-notify request to one with an empty body produced no response at all and left its client waiting on a reply that was never sent. Measured before the fix: 0 bytes out for an empty body, 50 for a good one. This is the failure mode #2732 set out to remove -- a well formed request going unanswered -- still reachable on the one endpoint kind that reads without a guard. An empty body now answers no_read_input, which is what a whitespace-only body has always answered. Notifications are the other half of the rule and were wrong in the opposite direction. The sender has said it will not read a reply, and the registry is careful about that everywhere else -- an unknown method in a notification is silently ignored. But read_params wrote a full error response for a notification with a malformed body, so a client that sent one got an answer it was never going to read. A failed read of a notification is now reported by not answering it, exactly as a successful one is. Both overloads, since a custom call handler reaches the state_view one directly.
Both doc comments promised that a false return means the error response was written. That stopped being true for a notification, which is now answered by silence whether the read succeeds or fails, so the comments asserted a guarantee the code no longer makes. A caller that returns immediately on false -- which is every registered endpoint -- is correct either way. One that inspects state.out afterwards has to allow for it being untouched, and now the comment says so.
The docs that landed in #2738 were written against the contract this branch changes: they say read_params writes the error response whenever it returns false, which stopped being true for notifications. A handler that trusted that sentence and appended to the response buffer would desynchronize the connection, which is the failure this branch exists to prevent. Both pages state the exception now, and the third pitfall spells out why the buffer can come back empty.
The registry echoes a request whose header already holds an error code back to its sender, and did so for notifications too. A notification is a request the sender has said it will not read a reply to, so that echo is not read as an answer to this request -- it is read as the answer to the next call, and the connection is desynchronized from there on. This is the rule the read_params change in this branch establishes, reached through a different path. The endpoint lookup above already drops an unknown method silently for exactly this reason. The header parsed and validated cleanly to reach this point, so its notify bit can be trusted. The malformed-header paths earlier in call() cannot make that claim and are left answering. Also drops "parse" from the read_params docs: an empty body is now answered with no_read_input, which is a failure but not a parse failure.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found while reviewing #2738. A non-notify REPE request with an empty body to a parameterized function endpoint gets no response at all — the client waits on a reply that is never sent.
This is the failure mode #2732 set out to remove, still reachable on one endpoint kind.
Why only that endpoint
read_paramsreported an empty body by returningfalsewithout writing anything. Every caller is told the error response is written whenever it returnsfalse, and every registered endpoint returns immediately on it, so nothing else wrote one either.Most endpoints never reach it — they guard on
state.has_body()and skip the read (repe_registry_impl.hpp:29,:107,:130). A parameterized function endpoint can't, since it has nothing to call its function with otherwise, so it reads unconditionally (:77). That is the one path where an empty body reachesread_params.An empty body now answers
no_read_input, which is what a whitespace-only body has always answered.The other half: notifications
Fixing only that would have made things worse in the opposite direction, so this covers both.
A notification is a request the sender has said it will not read a reply to, and the registry is careful about that everywhere else — an unknown method in a notification is silently ignored (
registry.hpp:455). Butread_paramswrote a full error response for a notification with a malformed body, so a client that sent one got an answer it was never going to read. A failed read of a notification is now reported by not answering it, exactly as a successful one is.Both overloads, since a custom call handler reaches the
state_viewone directly.Tests
registry_view_test: an empty body to a param function is answered withno_read_input; a good body still runs the function; a notification is not answered for an empty, malformed, or non-JSON body.Full suite 108/108 — nothing depended on notifications receiving error responses.
Note for #2738: its wording says the error response is written "on a parse failure", which was precise because of this bug. Once this lands that can be simplified to "on failure".