fix: let /status, /screenshot, and DELETE /session API methods to bypass the dispatch queue - #1222
Open
mykola-mokhnach wants to merge 10 commits into
Open
fix: let /status, /screenshot, and DELETE /session API methods to bypass the dispatch queue#1222mykola-mokhnach wants to merge 10 commits into
mykola-mokhnach wants to merge 10 commits into
Conversation
|
Since both this and #1214 seem to fix the same issue, are the changes complementary, or does this PR replace the other one? |
Author
They are complementary. This PR allows to take a screenshot, check status or delete the session without waiting for other requests to finish. Previously all requests were put into a single synchronous dispatch queue. The other PR tries to help all requests to not be stuck, so they don't deadlock the queue. |
KazuCocoa
reviewed
Aug 23, 2026
mykola-mokhnach
added a commit
that referenced
this pull request
Aug 23, 2026
- FBSession -kill: restore the self==_activeSession guard, but as an atomic check-and-clear under a lock this time. DELETE /session and session creation now genuinely run concurrently (both bypass the frozen route queue), so a session already superseded by a newer one can still reach -kill via a stale reference; without atomicity a belated call could win the race and null out the new session's pointer instead of its own. - FBHTTPServer: track session-scoped standalone requests (e.g. GET /session/:id/screenshot) in pendingSessionRequests too, not just non-standalone ones, so DELETE can abandon them the same way. DELETE /session's own request is excluded from tracking under its own session, since it's the one that performs the abandonment. - FBHTTPServer: track pending requests by a per-request identity object instead of by raw nw_connection_t, so two pipelined requests for the same session sharing one connection no longer collapse into a single tracked entry (which suppressed the second one's response permanently). - FBHTTPServer: allow only one in-flight request per connection at a time. Standalone routes now run on independent queues that can finish in any order, so without this, pipelined requests on one connection (e.g. /screenshot then /status) could have their responses written to the wire out of order.
KazuCocoa
reviewed
Aug 23, 2026
mykola-mokhnach
added a commit
that referenced
this pull request
Aug 23, 2026
- FBHTTPServer: move the buffer append itself onto bufferProcessingQueue, not just the parse. -client:didReceiveData: previously appended under a separate lock, so a receive callback could still mutate a connection's buffer while -processBufferForClient: was reading it unlocked on the processing queue. - FBSession: replace the per-instance kill-wait with class-level teardown-in-progress state, and add +killActiveSessionAndWaitForTeardown. The per-instance wait only helped a caller that still held a reference to the outgoing session; handleCreateSession: instead reads FBSession.activeSession fresh, so once a concurrent -kill had already cleared the pointer, it saw nil and proceeded to launch the replacement app without waiting for that -kill's teardown - including its app termination - to actually finish. - FBSession -fb_terminateTestedApplicationWithTimeout:: make the deferred main-thread -terminate call cancelable. If the bounded wait times out, the call is marked "given up on" under the same lock the deferred block checks before actually calling -terminate, so it can no longer fire later against a replacement session's app once -kill has reported its teardown finished. All FBSessionTests pass; WebDriverAgentLib builds clean.
KazuCocoa
approved these changes
Aug 23, 2026
…route queue When the app under test freezes, every route handler funnels through the shared main-queue dispatch before it can run, so even side-effect-free routes like /status wait behind a stuck handler indefinitely (#1210). Add a standalone route flag that skips the shared queue: concurrent requests to the same endpoint are coalesced into one in-flight execution, everything else gets its own queue. Mark /status, /screenshot, and DELETE /session standalone. Also fixes two related hang sources found while auditing these routes for their own risk of blocking: FBTestmanagerdVersion() and stopScreenRecordingWithUUID:error: used an unbounded wait on a daemon RPC, and -kill unconditionally cleared _activeSession even if a newer session had since become active.
…em stuck DELETE /session already bypasses the frozen route queue, but other in-flight requests for that same session didn't: a request queued or executing when the session dies would either hang until it finally got a turn, or (worse) run against a session mid-teardown. -kill now clears the active-session pointer and posts a notification up front, before its own teardown; FBWebServer uses it to immediately fail every pending request for that session with a proper W3C "invalid session id" error instead of leaving their clients waiting. A request already executing keeps running in the background - GCD can't abort it - but its result is discarded rather than ever reaching a client. Also bounds a related unbounded wait found while testing this: -kill's check for whether the tested app is the system app goes through a shared accessibility client that can itself be stuck behind another in-flight request against a frozen app. It's now capped at 5s, defaulting to "assume it might be the system app" (skip termination) on timeout to stay on the safe side.
+[XCUIApplication fb_systemApplication] is undocumented private API; its sibling -terminate is confirmed (this session, live) to hard-assert when called off the main thread, so the background-queue call added to bound -kill's system-app check needs the same @try/@catch already used around -terminate - an uncaught exception from inside a bare dispatch_async block has no handler and would crash the whole process, which is worse than the timeout this code already guards against.
Now that the clear happens up front instead of after -kill's teardown, the scenario the guard protected against - a slow -kill finishing after a newer session had already taken over - can't happen: every call site resolves self/_activeSession and calls kill() in the same uninterrupted synchronous chain (+sessionWithIdentifier: literally hands back _activeSession itself). The only way self != _activeSession at that point is an unsynchronized data race on the static, already an accepted, out-of-scope risk elsewhere in this design, and one the guard couldn't protect against anyway.
- FBSession -kill: restore the self==_activeSession guard, but as an atomic check-and-clear under a lock this time. DELETE /session and session creation now genuinely run concurrently (both bypass the frozen route queue), so a session already superseded by a newer one can still reach -kill via a stale reference; without atomicity a belated call could win the race and null out the new session's pointer instead of its own. - FBHTTPServer: track session-scoped standalone requests (e.g. GET /session/:id/screenshot) in pendingSessionRequests too, not just non-standalone ones, so DELETE can abandon them the same way. DELETE /session's own request is excluded from tracking under its own session, since it's the one that performs the abandonment. - FBHTTPServer: track pending requests by a per-request identity object instead of by raw nw_connection_t, so two pipelined requests for the same session sharing one connection no longer collapse into a single tracked entry (which suppressed the second one's response permanently). - FBHTTPServer: allow only one in-flight request per connection at a time. Standalone routes now run on independent queues that can finish in any order, so without this, pipelined requests on one connection (e.g. /screenshot then /status) could have their responses written to the wire out of order.
- FBSession -kill: dispatch [testedApplication terminate] to the main thread with a bounded wait instead of calling it inline. -kill can now run on a background queue (DELETE /session is standalone), and -terminate is confirmed to hard-assert off the main thread - the same reason the system-app check just above it is already background-dispatched. Waiting indefinitely for main would reintroduce the exact hang standalone routes exist to avoid if that's the queue currently stuck on another request, so give up after 5s and let the dispatched call finish on its own whenever main frees up. - FBSession -kill: a caller that loses the atomic active-session race now waits (bounded, via NSCondition) for the winner's teardown to actually finish before returning, instead of proceeding immediately. Session creation's own pre-kill of the outgoing session relies on this to not launch the new app while the old one's -terminate may still be in flight on another thread. - FBHTTPServer: route every call to -processBufferForClient: (from both new data arriving and a response completing) through one dedicated serial queue. The method parses a connection's buffer outside of any lock; that's only safe when no two calls for any connection can run concurrently, which no longer held once responses could complete on independent standalone-route queues and re-enter parsing directly on whichever thread finished the write. - FBHTTPServer: standalone-route coalescing now keys on the full path and query string instead of just the path, so a future standalone route that branches on query parameters can't have a second concurrent request silently served the first request's response. - FBXCodeCompatibility FBTestmanagerdVersion: stop caching the timeout fallback forever via dispatch_once. A merely-slow (not hung) first daemon reply would otherwise permanently lock in the fallback value for the rest of the process's life; only a real reply or the always-correct modern-testmanagerd branch is cached now, so a timeout is retried on the next call.
No behavior change - condenses several multi-line explanatory comments added in the prior two commits down to one or two lines each.
- FBConfiguration -bindingPortRange: collapse to a single return statement so the compiler can elide the copy (-Wnrvo). - FBTCPSocket -acceptConnection:: read weakSelf into a strong local before use, instead of a second direct weak read in the same block (-Warc-repeated-use-of-weak). - RouteResponse: mutableHeaders was declared `copy` on a mutable dictionary type, which would silently store an immutable object if ever assigned through the property setter; declare it `strong` instead, matching how it's actually used (osx.ObjCProperty).
- FBHTTPServer: move the buffer append itself onto bufferProcessingQueue, not just the parse. -client:didReceiveData: previously appended under a separate lock, so a receive callback could still mutate a connection's buffer while -processBufferForClient: was reading it unlocked on the processing queue. - FBSession: replace the per-instance kill-wait with class-level teardown-in-progress state, and add +killActiveSessionAndWaitForTeardown. The per-instance wait only helped a caller that still held a reference to the outgoing session; handleCreateSession: instead reads FBSession.activeSession fresh, so once a concurrent -kill had already cleared the pointer, it saw nil and proceeded to launch the replacement app without waiting for that -kill's teardown - including its app termination - to actually finish. - FBSession -fb_terminateTestedApplicationWithTimeout:: make the deferred main-thread -terminate call cancelable. If the bounded wait times out, the call is marked "given up on" under the same lock the deferred block checks before actually calling -terminate, so it can no longer fire later against a replacement session's app once -kill has reported its teardown finished. All FBSessionTests pass; WebDriverAgentLib builds clean.
Those vendored dependencies were removed when the HTTP server was unified on Network.framework; nothing in the tree references them anymore. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
3 participants
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.
Summary
Fixes #1210: when the app under test freezes, WDA's whole HTTP server stops answering — including routes like
GET /statusandDELETE /session/:idthat don't touch the frozen app at all — because every route handler is funneled through the same main-queue dispatch before it can run.standaloneroute flag that bypasses the shared route queue entirely. Concurrent requests to the same endpoint are coalesced into a single in-flight execution (its response is fanned out to all callers); everything else gets its own queue, so distinct standalone endpoints always run in parallel with whatever else is stuck. MarkGET /status,GET /screenshot, andDELETE /sessionstandalone."invalid session id"error instead of leaving their HTTP clients waiting on a session that no longer exists. A request that's already executing keeps running to completion in the background — GCD gives no way to abort it — but its result is discarded rather than ever reaching a client.-killnow clears the active-session pointer up front, before its own teardown, so a request that arrives mid-teardown correctly resolves to "no such session" instead of racing in against a half-torn-down one.FBTestmanagerdVersion()andstopScreenRecordingWithUUID:error:used an unbounded wait on a daemon RPC reply.-kill's check for whether the tested app is the system app goes through a shared accessibility client (FBXCAXClientProxy) that can itself be stuck behind another in-flight request against a frozen app — now capped at 5s, defaulting to "assume it might be the system app" (skip termination) on timeout to stay on the safe side. That check runs on a background queue now, guarded with@try/@catchsince a siblingXCUIApplicationmethod (-terminate) is confirmed to hard-assert off the main thread.