feat: add streamed response format 0.12.0 - #250
Conversation
The multipart response is serialised in full before any of it reaches the caller, so a site that renders progressively still arrives as one block once the runtime has finished. This adds a response format whose parts are length prefixed, letting a caller read them as they are framed. Content is prefixed with a hex length in the same shape HTTP uses for chunked transfer encoding, and each part is marked Content-Transfer-Encoding: chunked. The prefix is what makes the incremental read tractable on the other side: content is never scanned for the boundary, so a body carrying the boundary string cannot split the envelope and the reader needs no lookahead. statusCode and headers are known from the runtime's response headers before its first body byte, so they are emitted first and the body streams after them. logs, errors and duration are only known once the body is done, so they trail it. Streaming commits on the first body byte: a response with no body never commits and falls back to the buffered document, as does any caller below 0.12.0 or asking for JSON.
Asserts against the raw wire rather than a decoded body, because the framing is the thing being tested: that parts arrive length prefixed and chunk marked, that statusCode and headers land before the body, and that a body too large for one socket read is framed as several runs instead of one document. A single run would mean the executor buffered after all, so that assertion is what separates this from the existing execution tests. Adds a node resource that returns a body sized by request header to produce that case, and checks a 0.11.0 caller still receives the buffered document with no format echo.
|
Paired with appwrite/appwrite#13314, which implements the reader. Neither side does anything alone — that PR falls back to the buffered path until an executor emits this format, so this one should land first. |
Extract the regex capture into a helper that returns a string, so the offset access is typed rather than assumed, and take rector's instanceof narrowing and spacing.
e0fc7e2 to
fe24b50
Compare
The writer carried a twenty-two line class docblock and method docblocks that restated their own names, at roughly eight times the comment density of BodyMultipart beside it. Keep the wire format, the reason content is length prefixed, and the contracts that are not visible from a signature.
|
@greptile re-review |
Greptile SummaryAdds negotiated response format
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported transport-error and header-encoding paths are addressed before or immediately after response commitment as appropriate. Important Files Changed
Reviews (13): Last reviewed commit: "refactor: name the response format versi..." | Re-trigger Greptile |
A curl failure after the body has begun streaming throws out of createExecution, and the error hook then appends its JSON document to a response whose headers and content are already on the wire. The caller reads those bytes as envelope content, so the error leaks into the visitor-facing body before the truncated envelope is finally rejected. Once the first byte is out, close the connection instead: a broken transfer is the one remaining way to report the failure, and the caller already treats an envelope without its closing delimiter as a failed execution.
A header value that is not valid UTF-8 makes json_encode return false, which was written as a well-formed zero-length headers part. The caller then served the page with none of its headers. Encode before anything is written, so the failure surfaces while the response is still recoverable.
|
@greptile re-review |
The runner returned a 'streamed' key that the controller read once and then unset before serialising, while a local already tracked the same fact from the one place that can know it: whether any byte reached the write callback. That local is also the more accurate of the two, since a part that fails to encode throws before writing anything. isEnded() had no caller outside the assertion that read it.
Every caller already passes a closure, and mixed only existed because a property cannot be declared callable.
Removing the empty-run guard from writeContent left the suite green: a substring check for the orphaned content could not match, because the mutant frames a spurious terminator ahead of the next length prefix rather than raw content. Asserting the whole wire catches it, and matches how the neighbouring tests read. The two array_search checks restated indices the full order assertion above them already fixes. The chunk counter measured how curl handed bytes to the client, which is per socket read whether or not the executor streamed, so it held for a buffered response too.
An execution that dies mid-part never calls end(), and the caller reads a missing closing delimiter as a failed execution. Nothing may close the envelope on its behalf, so hold that: adding a destructor that finalises makes this fail.
A generic capture-with-assertion wrapper had no counterpart anywhere in the suite, and two of its three callers were extracting the boundary, which the code under test does with a plain explode.
Stream said nothing about direction, which is how the same name ended up on both this and the reader that consumes it. Every class here is a plain noun or names its role, as StorageFactory and ImagePuller do, and none is a Stream.
part() was the only bare noun among startPart, writeContent and endPart, and the document class beside it calls its equivalent setPart.
| // Once set, the response is committed: it can be neither retried nor turned into an error. | ||
| $streamed = false; | ||
|
|
||
| $executeV5 = function () use ($path, $method, $headers, $payload, $secret, $hostname, $timeout, $runtimeName, $logging, $onStream, &$streamed): array { |
There was a problem hiding this comment.
headersCallbakc, bodycallback
| $logging, | ||
| $restartPolicy, | ||
| ); | ||
| $responseFormat = $request->getHeaderLine('x-executor-response-format') ?: '0.10.0'; // Last version without support for array value for headers |
There was a problem hiding this comment.
Something here feels complex
There was a problem hiding this comment.
check OPR tests, probably copy over here
There was a problem hiding this comment.
avoid this class if doable
One callback tagged with an event string meant the payload type changed with the tag, so it had to be typed mixed and unpacked by the receiver. Two callbacks carry their own types: an int status with an array of headers, and a string chunk.
The class was five methods and two guards around what is, written out, three string expressions: a part header, a length prefixed run, and the zero length run that closes a part. The guards existed to keep a reusable object honest about its own state; inline, the call order is fixed and readable in one place, so they are not needed. Boundary generation still comes from BodyMultipart, so that logic stays in one place. The rule worth keeping in mind is that an empty write emits no run at all, since a zero length run is what closes a part.
The negotiation compared against three protocol versions with only the newest named, leaving '0.10.0' explained by a trailing comment and '0.11.0' bare. Name all three next to each other so the protocol history reads in one place and each comparison says what it gates.
Summary
Adds response format
0.12.0, whose multipart parts are length prefixed so a caller can read them as they are framed rather than waiting for the whole document to be serialised.This is the executor half of appwrite/appwrite#13314. That PR already implements the reader, but no executor emits this format, so it currently falls back to the buffered path on every request. Neither side does anything on its own.
The format
The hex prefix is the same shape HTTP uses for chunked transfer encoding. It is what makes the incremental read tractable: content is never scanned for the boundary, so content carrying the boundary string cannot split the envelope and the reader needs no lookahead between socket reads.
statusCodeandheaderscome from the runtime's response headers, which are complete before its first body byte, so they are emitted first and the body streams after them.logs,errors,durationandstartTimeare only known once the body is done, so they trail it.Compatibility
Streaming commits on the first body byte. Until then nothing has been written, so:
0.12.0gets the buffered document, unchanged, with no format echoThe retry loop is guarded: once content is on the wire a second attempt would append a second response to it. The retryable errors all occur before a connection is established, so this is a guard rather than an expected path.
Failure handling
Once the first body byte is out the response is committed: the status line is already on the wire and cannot be revised.
json_encodereturnfalse. That now throws before anything is written, while the response is still recoverable, rather than emitting a well-formed but emptyheaderspart that would leave the caller serving a page with none of its headers.Prior art
Supersedes #223 and appwrite/appwrite#11429 (both closed unmerged, "Towards SER-334"). Those detected
text/event-streamand forwarded the raw body with no multipart envelope, which meant losinglogs,errorsandduration. Keeping the envelope preserves execution metadata.Tests
tests/unit/Executor/BodyMultipartStreamTest.php— 14 tests covering framing, hex prefixing, empty runs (a zero length run would read as the part terminator), boundary-in-content, binary content, and that an abandoned envelope is never closed on the execution's behalftests/e2e/StreamedResponseTest.php— three cases: envelope framing and part ordering, a 1MB body framed as several runs rather than one, and a0.11.0caller still receiving the buffered documentnode-large-responsefixture returning a body sized by request header, to produce the multi-run caseUnit, format, analyze, refactor and e2e all pass in CI.
Verified separately against a real socket with real curl, wiring this writer directly to the reader in appwrite/appwrite#13314 — both production classes, no mocks. Against a server dribbling a run every 250ms,
statusCodeandheadersparse at +1ms and body runs arrive at +255/509/764/1018ms, with the trailing parts after. The body is genuinely forwarded as produced rather than reassembled, and the two implementations agree on the wire.Note on scope
Sites (SSR) are framework servers that produce output progressively, so they benefit. Functions are buffered inside every runtime, so this does not make them stream.