Fix single-AsyncIterator crash in multi-frame gRPC request handlers - #955
Fix single-AsyncIterator crash in multi-frame gRPC request handlers#955erneestoc wants to merge 1 commit into
Conversation
|
Hi @erneestoc! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
grpc-swift's GRPCAsyncRequestStream is backed by
NIOThrowingAsyncSequenceProducer, which fatal-errors if more than one
AsyncIterator is ever created from it:
NIOThrowingAsyncSequenceProducer allows only a single AsyncIterator
to be created
The AsyncSequence.requiredNext helper reads via first(where:), and every
first(where:) call makes a new iterator. `for try await ... in requestStream`
likewise makes one. Any handler that combines two such reads on the same
request stream therefore crashes the whole companion on the second read.
This affects, directly or via a shared helper:
- install (destination + payload frames)
- record / `idb video` (start, stop)
- launch --wait-for (start, stop)
- tail (start, stop)
- xctrace record (start, stop)
- instruments run (start, stop)
- dap (start, then consumeElements loop)
- repl (start, then serve loop)
- video-stream (start, then client-cancel loop)
- push / add-media (via MultisourceFileReader, which reads
requiredNext then loops the remaining frames)
Introduce a shared SingleIteratorRequestStream that owns one iterator per
RPC, and route every multi-frame handler's reads through it. Handlers that
delegate to a helper (dap/repl/push/add-media) thread the wrapper into the
helper; MultisourceFileReader now takes the wrapper too. The trailing
`for try await request in requestStream` loops become
`while let request = try await stream.next()` on the same iterator.
Single-read handlers keep AsyncSequence.requiredNext, now with a
doc-comment warning about the single-shot hazard.
Reproduced with grpc-swift 1.23.1 / current SwiftNIO: the companion aborts
the moment any of these RPCs reads its second frame.
52e4e23 to
5eef451
Compare
…handlers The build patch previously wrapped only Install and Record. grpc-swift's GRPCAsyncRequestStream fatal-errors on a second AsyncIterator, which every handler that reads more than one request frame hits -- via requiredNext twice, or requiredNext plus a `for try await ... in requestStream` loop. Extend the shared SingleIteratorRequestStream to Launch, Tail, Xctrace, Instruments, Dap, Repl, and VideoStream, and refactor MultisourceFileReader to take it too (which is what made Push and AddMedia crash). Move the wrapper into its own Companion/Utility/SingleIteratorRequestStream.swift and add a doc-warning to AsyncSequence.requiredNext. Upstreamed as facebook/idb#955.
|
@lawrencelomax been working on rules_idb for bazel to allow me to run more than a few concurrent simulators for my test runs (xcodebuild test-without-building is using too much memory for some reason). Let me know if there's something I can help with. |
1 participant
Summary
GRPCAsyncRequestStreamis backed byNIOThrowingAsyncSequenceProducer, which fatal-errors if more than oneAsyncIteratoris ever created from it:The
AsyncSequence.requiredNexthelper (Companion/Utility/AsyncSequence+Extension.swift) reads viafirst(where:), and everyfirst(where:)call constructs a fresh iterator; afor try await … in requestStreamloop makes one too. Any handler that combines two such reads on the same request stream aborts the whole companion on the second read.Affected RPC handlers (directly, or via the shared
MultisourceFileReader):InstallMethodHandlerfor try await … in requestStreamidb installRecordMethodHandleridb video/ screen recordingLaunchMethodHandleridb launch --wait-forTailMethodHandlerXctraceRecordMethodHandleridb xctrace recordInstrumentsRunMethodHandleridb instrumentsDapMethodHandlerconsumeElementsloopidb dapReplMethodHandlerserveloopidb replVideoStreamMethodHandleridb video-streamPushMethodHandler/AddMediaMethodHandlerMultisourceFileReaderreadsrequiredNextthen loops the restidb file push,idb add-mediaFix
Introduce a shared
SingleIteratorRequestStream(Companion/Utility/) that makes one iterator per RPC and exposesnext()/requiredNextagainst it. Each multi-frame handler wraps itsrequestStreamonce and routes every read through the wrapper; handlers that delegate to a helper (dap,repl,push,add-media) thread the wrapper into the helper, andMultisourceFileReadernow takes the wrapper as well. The trailingfor try await request in requestStreamloops becomewhile let request = try await stream.next()on the same iterator.The
requiredNextextension is kept for the many genuine single-read handlers, now with a doc-comment warning about the single-shot hazard.Notes
idb videonow records a valid MP4 instead of aborting.debugserver,hid,video-stream-single-frame paths, etc.).