feat(rpc): stream ActiveSync Sync response bodies#5
Conversation
When constructed with the new 'streaming' parameter, Horde_Rpc_ActiveSync skips the 1 MiB output buffer for Cmd=Sync POST requests, disables zlib compression, and sends the body without Content-Length so the webserver applies chunked transfer-encoding and the Sync handler can flush WBXML to the client incrementally. All other commands (GetAttachment, ItemOperations, Ping, ...) keep the buffered Content-Length response (Bug #12486). HTTP 400/500 error headers are guarded with headers_sent() because the status line cannot be changed once body bytes are on the wire. Refs horde/ActiveSync#83, horde/ActiveSync#77.
There was a problem hiding this comment.
Pull request overview
This PR adds an opt-in streaming response path for ActiveSync Sync (POST) requests in Horde_Rpc_ActiveSync, intended to let clients receive incremental WBXML bytes (via chunked transfer encoding) rather than waiting for the full buffered response and a Content-Length header. This aligns the RPC layer with handler-side flushing work referenced in the linked ActiveSync PR/issue.
Changes:
- Add a streaming toggle (
$params['streaming']) and decision logic to enable streaming only forCmd=Sync+POST. - When streaming, disable zlib output compression and avoid the 1 MiB output buffer/
Content-Lengthpath. - Add a unit test covering the streaming decision matrix and the early-return behavior in
sendOutput().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/Horde/Rpc/ActiveSync.php | Adds streaming mode decision + streaming-specific output/header behavior for Sync POST requests. |
| test/Unit/ActiveSyncStreamingTest.php | Adds unit tests for the streaming decision logic and the streaming sendOutput() early return. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if ($this->_streaming) { | ||
| // Compression would buffer the body and defeat streaming. | ||
| @ini_set('zlib.output_compression', 0); | ||
| while (ob_get_level()) { | ||
| ob_end_flush(); | ||
| } | ||
| $this->_logger->debug('Horde_Rpc_ActiveSync: streaming response body for Sync.'); | ||
| } else { | ||
| ob_start(null, 1048576); | ||
| } |
There was a problem hiding this comment.
Fixed in 9bac792 (first part): pre-existing buffers are now discarded with @ob_end_clean() instead of flushed, and the loop breaks when a buffer refuses to end (non-removable buffers would otherwise loop forever, since a failed ob_end_flush() does not decrement the level). Discarding is correct here — anything sitting in a stray buffer at this point (whitespace from includes, notices) must not precede the WBXML stream.
The Content-Type part is a non-issue: Horde_ActiveSync::handleRequest() sends the response headers — including Content-Type: application/vnd.ms-sync.wbxml via contentTypeHeader() — before dispatching the Sync handler (it must, because some handlers such as GetAttachment start emitting output immediately; see Bug #12486). So headers are always on the wire before the first streamed body byte, in both buffered and streaming modes.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Discard pre-existing output buffers with ob_end_clean() instead of flushing them (stray buffered bytes must not precede the WBXML stream) and stop on non-removable buffers instead of looping. Set the PHPUnit output expectation before invoking sendOutput() and repair the truncated test method from the previous autofix commit.
Release version 3.1.0 Merge pull request #5 from horde/feat/sync-response-streaming fix(rpc): address streaming review findings Potential fix for pull request finding feat(rpc): stream ActiveSync Sync response bodies
Summary
Syncresponse delivery: forCmd=SyncPOST requests only,Horde_Rpc_ActiveSyncskips the full-response output buffer, disables zlib output compression, and sends noContent-Lengthheader, so the web server delivers the handler's incrementally flushed WBXML via chunked transfer-encoding.Motivation
Some clients — notably Gmail on Android — abort a
Syncconnection after ~30 seconds without response body bytes, retry with the old sync key, and can end up in a broken sync state. Historically this layer buffered the entire response (1 MiB output buffer,Content-Length), so the client saw nothing until the handler had finished all work. Streaming keeps bytes flowing while the handler works; the protocol-level flushing lives inhorde/activesync.Changes
Horde_Rpc_ActiveSyncaccepts the$conf['activesync']['sync']['streaming']flag (passed through byhorde/horderpc.php, already merged intoFRAMEWORK_6_0).Cmd=SyncPOST requests. All other commands (GetAttachment,ItemOperations,Ping, OPTIONS, Autodiscover, …) keep the bufferedContent-Lengthresponse unchanged, as doesSyncwhen the flag is off (default).zlib.output_compressionis disabled for the request, response headers are sent before the handler runs, and output is flushed as the handler produces it.header()calls withheaders_sent(): an error after the first flushed byte degrades to a truncated (prefix-valid) WBXML response the client simply re-requests — never a mid-stream HTTP status change.test/Unit/ActiveSyncStreamingTest.phpcovers the streaming decision logic (command/method/flag matrix) and header behaviour.Test plan
Content-Length.Merge together with horde/ActiveSync#84 — the streaming flag is inert without the handler-side flushing, and vice versa.