Robustness modifications to server - #18
Open
benhar-dev wants to merge 4 commits into
Open
Conversation
Two defects found while reviewing the session and multi-step request
handling that mobject-client drives.
Reject follow-up requests whose session has gone.
SessionsMiddleware treated an expired or unknown session exactly like a
first contact: it silently minted a new session and let the request
continue. For a follow-up (a request carrying a Response-Id, i.e. an
async poll or a chunk fetch) the state being referred to lived in the
session that just vanished, so the lookups in ChunkedResponseManager and
AsyncResponseManager miss. Both simply return on a miss, so the request
fell through to RpcMiddleware, which still saw the original Method-Name
on the header and ran the handler a second time. A poll for a graph
execution arriving after the 10s session timeout re-ran the graph.
Such follow-ups are now rejected with SESSION_EXPIRED. A fresh session is
still issued on the error response so the client adopts it and its next
request succeeds. First-contact requests keep the previous behaviour.
Correct the payload size guards that ignored the null terminator.
SendNextChunk reserves the last byte of the buffer for the null
terminator, but three guards compared against the full buffer size:
- ChunkedResponseManager.TryHandleIncomingRequest decided whether more
chunks follow. With exactly 8001 bytes remaining the client was told
it had the last chunk while only 8000 were sent, silently dropping a
byte. Hit any payload of 8000k + 8001 bytes.
- ChunkedResponseManager.HandleSuccessWithPayload let an 8001 byte
payload go out unchunked, filling a STRING(8000) with no room for the
terminator.
- ServerResponse.SuccessWithPayload accepted 8001 data bytes for the
same reason, and reported the wrong maximum in its error message.
All three now use the transmittable data size (buffer size - 1).
Adds a regression test covering the re-execution case. These changes are
untested beyond review: no TwinCAT toolchain was available.
An asynchronous handler which never called Complete or Reject left its response pending forever. Nothing reclaimed it: the client kept polling, and every poll refreshed the session, so the inactive-session sweep that would otherwise have collected it never fired. The call hung until something unrelated disposed the underlying work. A pending response now records when it was created and is given a deadline. When a poll arrives for one that has outlived it, the response fails with HANDLER_TIMEOUT and is discarded rather than answered with another Pending. The deadline is ServerConfiguration.PENDING_RESPONSE_MAX_AGE, so a project can tune it the same way it tunes the payload size. It defaults to 60s, and T#0S switches it off. HasExpired is deliberately a statement about age alone - the manager only consults it while a response is still pending, so a completed response stays collectable however old it is. No sweep is needed for a client that simply walks away. That case was already covered: without polls the session stops being refreshed, expires, and disposes everything it holds.
Both found while reviewing the earlier session fix, which only covered follow-ups carrying a Response-Id. A chunked upload carries a Request-Id and a chunk sequence instead, so it took neither of the paths that fix guarded. A chunk arriving with nothing stored for it was treated as the start of a new upload, whatever its sequence number. If the session holding the earlier chunks expired part way through, the server quietly opened a fresh buffer, dropped everything already sent, and waited for a chunk total that could no longer be reached. The client got no error - it simply waited until its own timeout. A chunk other than the first with nothing stored now fails with CHUNKED_REQUEST_LOST so the request can be sent again. Separately, TryAddChunkedPayload validated the sequence number and then ignored it, appending every chunk in arrival order. Anything out of order or repeated was appended anyway, silently corrupting the assembled request and miscounting completion. Placing by index is not possible here - the buffer is sized by the server's own chunk size while the client chunks to a slightly smaller stride - so the sequence is now required to be the one due next, which is what append already assumed.
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
1 participant
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.
No description provided.