fix(ios): don't start a media upload for a torn-down editor - #626
Draft
jkmassel wants to merge 1 commit into
Draft
fix(ios): don't start a media upload for a torn-down editor#626jkmassel wants to merge 1 commit into
jkmassel wants to merge 1 commit into
Conversation
Both delivery paths could put bytes on the wire after the editor was gone. `EditorViewController.deinit` calls `stop()`, which cancels the in-flight connection tasks, but Swift cancellation is cooperative: the body read is an uninterruptible loop and a host's `processFile` need not check at all, so a request can reach delivery well after teardown. Whether it then actually reached WordPress rested entirely on URLSession noticing the cancellation. That is not a guarantee the server can rely on. `URLSessionProtocol` is public and documented for dependency injection, and the obvious conformance for a host wrapping a callback-based stack — `withCheckedThrowingContinuation` around a completion handler — has no cancellation awareness at all. Such a host would upload deterministically after teardown, and the response is discarded either way, leaving an attachment on the site that nothing cleans up. Check cancellation explicitly before delivery in `processAndUpload` and before the passthrough forward, so the guarantee comes from this file rather than from the HTTP client's behavior. `uploadErrorResponse` already logs CancellationError quietly, and HTTPServer drops the response for a cancelled task. Not covered by a test: reaching the window deterministically means driving teardown between the parse and the delivery of a live socket request, and a timing-based approximation would be flaky without pinning the behavior.
4 tasks
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/626")Built from c51eaf6 |
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.
Stacked on #625. Third of ten PRs splitting #621.
What?
Both delivery paths could put bytes on the wire after the editor was gone. Check cancellation explicitly before delivery.
Why?
EditorViewController.deinitcallsstop(), which cancels the in-flight connection tasks, but Swift cancellation is cooperative: the body read is an uninterruptible loop and a host'sprocessFileneed not check at all, so a request can reach delivery well after teardown. Whether it then actually reached WordPress rested entirely on URLSession noticing the cancellation.That is not a guarantee the server can rely on.
URLSessionProtocolis public and documented for dependency injection, and the obvious conformance for a host wrapping a callback-based stack —withCheckedThrowingContinuationaround a completion handler — has no cancellation awareness at all. Such a host would upload deterministically after teardown, and the response is discarded either way, leaving an attachment on the site that nothing cleans up.How?
MediaUploadServer.swift:
try Task.checkCancellation()inprocessAndUploadbefore delivery, and before the passthrough forward. The guarantee now comes from this file rather than from the HTTP client's behavior.uploadErrorResponsealready logsCancellationErrorquietly, andHTTPServerdrops the response for a cancelled task.Testing Instructions
Not covered by a test: reaching the window deterministically means driving teardown between the parse and the delivery of a live socket request, and a timing-based approximation would be flaky without pinning the behavior. Called out rather than faked.
swift test— host suite green