TestApp: graceful quit; debugger integration#383
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors POSIX signal handling in both ChangesSignal handling and app lifecycle management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@appium/entry.swift`:
- Around line 4-13: The signalHandler function and the signal() calls in
applicationDidFinishLaunching are using POSIX signal handlers to perform
non-async-signal-safe operations like Task creation and NSApp.terminate(), which
violates safety rules. Replace the signal(SIGINT, signalHandler) and
signal(SIGTERM, signalHandler) calls with DispatchSourceSignal instances
configured on the main dispatch queue. Create separate DispatchSourceSignal
objects for SIGINT and SIGTERM, set their event handler to perform the
NSApp.terminate(nil) operation directly on the main queue (without the Task
wrapper), and resume each source. Remove or repurpose the signalHandler function
since it will no longer be needed as a POSIX signal callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c7dfa42b-c230-428f-99bb-66f0f4e604a5
📒 Files selected for processing (2)
.vscode/launch.jsonappium/entry.swift
| private func signalHandler(signal: Int32) { | ||
| Task { @MainActor in | ||
| NSApp.terminate(nil) | ||
| } | ||
| } | ||
|
|
||
| class TestAppDelegate: NSObject, NSApplicationDelegate { | ||
| func applicationDidFinishLaunching(_ notification: Notification) { | ||
| signal(SIGINT, signalHandler) | ||
| signal(SIGTERM, signalHandler) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
POSIX async-signal-safe rules for signal handlers, and Apple guidance for handling SIGINT/SIGTERM in macOS apps with DispatchSourceSignal.
💡 Result:
POSIX defines a specific set of functions that are async-signal-safe, meaning they can be safely called from within a traditional signal handler [1][2][3]. A function is async-signal-safe if it is either reentrant or atomic with respect to signals [1][4]. If a signal handler interrupts an unsafe function and then calls another unsafe function, the program's behavior is undefined [2][3]. The list of POSIX-mandated async-signal-safe functions includes, but is not limited to, _exit, close, getpid, kill, open, read, write, and sigprocmask [1][3]. Notably, common functions like malloc, printf, and exit are not async-signal-safe and should never be called inside a signal handler [5][3]. For macOS applications, Apple recommends using DispatchSourceSignal as the preferred mechanism for handling signals like SIGINT or SIGTERM instead of traditional synchronous signal handlers [6][5]. Key guidance for macOS applications: 1. Avoid Synchronous Handlers: Traditional signal handlers (via sigaction) run in a highly restricted environment where almost any non-trivial work is unsafe [5]. 2. Use DispatchSourceSignal: This GCD-based approach allows you to monitor signal arrival asynchronously on a dispatch queue [6][7]. Because the event handler executes on a dispatch queue rather than interrupting the current thread, it is not subject to async-signal-safety restrictions [8][5]. 3. Implementation Steps: - Use signal or sigaction to ignore the signal (e.g., signal(SIGTERM, SIG_IGN)) so that the process is not terminated immediately [6][8][5][9]. - Create a dispatch source of type DISPATCH_SOURCE_TYPE_SIGNAL for the desired signal [7][8][5]. - Set an event handler on that source to perform your cleanup logic [6][5]. - Call dispatch_resume to start monitoring [6][9]. 4. Limitations: Dispatch sources cannot monitor hardware-generated exceptions like SIGILL, SIGBUS, or SIGSEGV [6]. They are intended for process notification, not as a replacement for low-level fault handling [6]. Top results: [1][6][8][5][3]
Citations:
- 1: https://man7.org/linux/man-pages/man7/signal-safety.7.html
- 2: https://pubs.opengroup.org/onlinepubs/9699959099/functions/xsh_chap02_04.html
- 3: https://cmu-sei.github.io/secure-coding-standards/sei-cert-c-coding-standard/rules/signals-sig/sig30-c/
- 4: https://manpages.ubuntu.com/manpages/bionic/man7/signal-safety.7.html
- 5: https://developer.apple.com/forums/thread/44221
- 6: https://developer.apple.com/library/archive/documentation/General/Conceptual/ConcurrencyProgrammingGuide/GCDWorkQueues/GCDWorkQueues.html
- 7: https://developer.apple.com/documentation/dispatch/dispatch_source_type_signal
- 8: https://keith.github.io/xcode-man-pages/dispatch_source_set_event_handler_f.3.html
- 9: https://stackoverflow.com/questions/45713819/trapping-signals-in-a-swift-command-line-application
Replace signal() callback with DispatchSourceSignal on the main queue.
The current handler performs non-async-signal-safe work—Task creation and NSApp.terminate(nil)—from a POSIX signal context. This violates async-signal-safety rules and can cause deadlock or crash on SIGINT/SIGTERM. Apple recommends using DispatchSourceSignal to defer signal handling to the main dispatch queue, where unsafe operations are permitted.
Suggested fix
-private func signalHandler(signal: Int32) {
- Task { `@MainActor` in
- NSApp.terminate(nil)
- }
-}
-
class TestAppDelegate: NSObject, NSApplicationDelegate {
+ private var sigintSource: DispatchSourceSignal?
+ private var sigtermSource: DispatchSourceSignal?
+
+ private func installSignalHandlers() {
+ signal(SIGINT, SIG_IGN)
+ signal(SIGTERM, SIG_IGN)
+
+ sigintSource = DispatchSource.makeSignalSource(signal: SIGINT, queue: .main)
+ sigtermSource = DispatchSource.makeSignalSource(signal: SIGTERM, queue: .main)
+
+ sigintSource?.setEventHandler { NSApp.terminate(nil) }
+ sigtermSource?.setEventHandler { NSApp.terminate(nil) }
+
+ sigintSource?.resume()
+ sigtermSource?.resume()
+ }
+
func applicationDidFinishLaunching(_ notification: Notification) {
- signal(SIGINT, signalHandler)
- signal(SIGTERM, signalHandler)
+ installSignalHandlers()
start_fcitx_thread("")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@appium/entry.swift` around lines 4 - 13, The signalHandler function and the
signal() calls in applicationDidFinishLaunching are using POSIX signal handlers
to perform non-async-signal-safe operations like Task creation and
NSApp.terminate(), which violates safety rules. Replace the signal(SIGINT,
signalHandler) and signal(SIGTERM, signalHandler) calls with
DispatchSourceSignal instances configured on the main dispatch queue. Create
separate DispatchSourceSignal objects for SIGINT and SIGTERM, set their event
handler to perform the NSApp.terminate(nil) operation directly on the main queue
(without the Task wrapper), and resume each source. Remove or repurpose the
signalHandler function since it will no longer be needed as a POSIX signal
callback.
arm64 comparisonNo difference. x86_64 comparisonNo difference. |
1 participant
Summary by CodeRabbit