Repository navigation
Conversation
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/814")Built from 312a1c7 |
7 tasks done
jkmassel
force-pushed
the
jkmassel/upload-writer-never-waits
branch
2 times, most recently
from
October 19, 2026 18:54
56b3ab5 to
bb83201
Compare
…pload Every `MultipartBodyWriter` shares one serial queue, so no write may block. Two things let a write block it. `start()` opened the stream on the caller's thread after handing it to the queue, so the stream could deliver an event to the queue while `open` was still running. The two threads' calls into the stream could leave it stuck opening, and a write then waited in `waitForOpen` forever. That hung two `EditorViewControllerMediaTeardownTests` upload tests on CI, in 4 of 21 runs on 2026-10-19: the stuck writer was a test's, writing to `OutputStream.toMemory()`, and every writer behind it on the queue stopped. A stress run on an iPhone 15 Pro reproduced it. The writer also wrote on every "room available" event without checking, but an event doesn't promise room. Writing to a full bound pair, which upload bodies use, blocks until its reader reads. That hasn't been seen in an app. The writer now opens the stream on the queue, writes only when the stream says it can take bytes, and also writes on `.openCompleted`. New tests check that a writer whose stream is full leaves the queue free, that `.openCompleted` writes, and that `start()` opens the stream on the queue. The queue is internal so the tests can use it.
jkmassel
force-pushed
the
jkmassel/upload-writer-never-waits
branch
from
October 19, 2026 21:21
bb83201 to
2623b72
Compare
dcalhoun
approved these changes
Oct 20, 2026
dcalhoun
left a comment
Member
There was a problem hiding this comment.
This is a draft, but my review is requested, so I presumed the draft state was erroneous. This looks good.
I captured one finding by Claude worth considering. I could be misguided.
`writerNeverHoldsUpTheSharedQueue` waited for the stream's buffer to fill by calling `queue.sync` inside `waitUntil`. `waitUntil` checks its deadline only between calls. So if the queue was stuck, `queue.sync` never returned, and the run hung without naming this test. The check now runs on the queue with `async` and reports back through a lock, so a stuck queue fails this wait after 60 s. If the stuck write is this test's own, the `defer` then frees the queue, and other tests waiting on it carry on. A queue stuck by anything else stays stuck, and tests that wait on a writer with no timeout still hang the run. The check also requires the stream to be open. A closed stream also reports no room, so a writer that closed its stream early would have passed the old wait, and the test would have checked nothing. Co-authored-by: David Calhoun <github@davidcalhoun.me>
jkmassel
added a commit
that referenced
this pull request
Oct 20, 2026
`writerOpensOnTheQueue` held the shared writer queue on a semaphore until the test's own task ran again. Other tests in the suite read a body with a blocking read on a Swift concurrency thread, and those reads wait on that queue. When they took every thread, the test's task could never run, so the hold was never released. The whole run then hung without naming a test. With the thread pool narrowed to 4 threads, 4 of 5 runs hung this way. With this test skipped, none did. The test no longer holds the queue. It calls `start` from a block on the queue and reads the stream's status in that same block. The writer queues its own `open` behind the block, so the status still shows whether `start` opened the stream itself, and nothing waits on anything.
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.
Fixes a hang where one upload body could stall every other upload that GutenbergKit's own media client sends. On CI, it hung two
EditorViewControllerMediaTeardownTestsupload tests until the job was cancelled, in 4 of 21 iOS Simulator Tests runs on 2026-10-19:It cost #804 a run (build 3286) and three of #813's soak runs (build 3287).
This targets #625's branch, where
MultipartBodyWriterarrived with #747.Who is affected
The CI hang came from a stream type only the tests use, so only test runs have hung.
Apps can hit a related problem. It applies to hosts that don't supply their own media upload handler, so uploads go through
InternalMediaClient. If a "room available" event reaches a writer whose buffer is already full, the old writer waits for that upload's reader, and every other upload waits with it. We have not seen this in an app, and haven't reproduced it end to end.Root Cause
Every
MultipartBodyWritershares one serial queue. That is safe only if no write blocks. Two things let a write block it.1. The stream was opened off the queue
start()handed the stream to the queue, then calledopen()on the caller's thread. The stream could deliver its first event to the queue whileopen()was still running. The two threads' calls into the stream could leave it stuck "opening", and a write then waited inwaitForOpenforever.A sample of a hung CI run (build 3290, from a throwaway branch) showed:
CFWriteStreamWrite→waitForOpenIn that job's log, "the writer sends the full body and closing boundary when the file reads cleanly" never finished. That test writes to
OutputStream.toMemory(); the other memory-stream test passed in 1.6 s. The upload tests hung because they share the queue it blocked. A stress run on an iPhone 15 Pro reproduced the hang: with the old writer, the shared queue was held for 25 s in the first round.2. The writer wrote without checking for room
The writer wrote on every "room available" event, but an event doesn't promise room: one can be left over from before the last write filled the buffer. Writing to a full bound pair, which upload bodies use, blocks until the reader reads.
Why the stuck writer wasn't an upload's
CFWriteStreamWritenever reacheswaitForOpenfor one. A memory stream has no such check, and signals room from inside its open.Fix
ios/Sources/GutenbergKit/Sources/Media/MultipartBodyWriter.swift:
start()has run, only the queue calls into the stream.output.hasSpaceAvailablesays the stream can take bytes; otherwise do nothing. With any room at all, a bound pair'swritetakes what fits and returns at once..openCompleted, so an event skipped because the stream hadn't finished opening is retried.queueis now internal (wasprivate), so the tests can use it.Test plan
start()it fails every time (3 of 3); with the fix it passed 10 of 10..openCompletedchange it fails; with it, it passes.URLSessionuploads to a local server that trickles, stalls or hangs up. Every body is checked byte for byte.