feat: modernize signal handling via Panama FFM sigaction()#1750
Merged
feat: modernize signal handling via Panama FFM sigaction()#1750
Conversation
Add provider-delegated signal handling so each TerminalProvider can implement signal registration using the best mechanism available. The FFM provider now uses native sigaction() with SA_RESTART instead of reflection-based sun.misc.Signal access. Key changes: - Add registerSignal/registerDefaultSignal/unregisterSignal default methods to TerminalProvider (delegates to Signals.java) - Create FfmSignalHandler with platform-specific sigaction() bindings for macOS, Linux, and FreeBSD, using an atomic-flag dispatcher for signal safety - Override signal methods in FfmTerminalProvider with automatic fallback to sun.misc.Signal when FFM signals are unavailable - Modify PosixSysTerminal to accept an optional TerminalProvider and delegate signal operations through it - Pass provider reference from all terminal providers (FFM, JNI, Exec) Closes #1747
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a Panama FFM-based POSIX signal handler implementation and provider-level signal APIs, wires the FFM-capable provider into PosixSysTerminal, and retains fallback to the existing Signals path when FFM registration is unavailable. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Posix as PosixSysTerminal
participant Provider as TerminalProvider
participant Ffm as FfmSignalHandler
participant Dispatcher as DispatcherThread
participant OS as POSIX (sigaction)
App->>Posix: handle(signal, handler)
Posix->>Provider: registerSignal(name, handler)
alt Provider uses FFM
Provider->>Ffm: register(name, handler)
Ffm->>OS: sigaction(signum, act, oldAct)
OS-->>Ffm: oldAction
Ffm-->>Provider: Registration(signum, oldAction)
else Fallback to Signals
Provider->>Provider: Signals.register(name, handler)
Provider-->>Posix: registration
end
OS->>Ffm: deliver signal (upcall)
Ffm->>Ffm: mark pending (atomic)
Dispatcher->>Ffm: poll pending flags
Ffm-->>Dispatcher: pending signum(s)
Dispatcher->>App: invoke registered handler(s)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java (1)
231-243: Minor:Arena.global()allocations persist for JVM lifetime.Each
register()call allocatesoldActandnewActinArena.global(), which are never freed. For typical terminal usage (register once, unregister on close), this is negligible. However, in long-running applications that create/destroy many terminals, this could accumulate.Given that signal structs are small (~100-200 bytes) and the number of signals is bounded, this is acceptable for the use case. If more aggressive cleanup is needed later, consider using a confined arena tied to the
Registrationlifecycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java` around lines 231 - 243, The code currently allocates oldAct and newAct with Arena.global() inside register(), causing allocations to persist for the JVM lifetime; change allocation to use a confined arena (e.g., Arena.openConfined() / a non-global MemorySession) for the signal structs, attach that arena to the returned Registration (store it as a field on Registration created in register() ), and ensure the arena is closed when the Registration is unregistered/disposed (add closing logic in Registration.close/unregister method) so the memory is reclaimed with the Registration lifecycle; update references in register() (oldAct, newAct, and returned new Registration(signum, oldAct)) and Registration class to manage the arena.terminal/src/main/java/org/jline/terminal/impl/PosixSysTerminal.java (1)
92-122: Consider extracting configuration into a builder or options object.The constructor now has 9 parameters, exceeding common complexity thresholds. While this is acceptable given the existing pattern and the need for backward compatibility, consider introducing a builder or configuration object in a future refactor to improve maintainability.
The signal registration logic (lines 111-119) correctly uses the new helper methods for provider-aware routing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal/src/main/java/org/jline/terminal/impl/PosixSysTerminal.java` around lines 92 - 122, The PosixSysTerminal constructor has grown to 9 parameters making it hard to maintain; extract related parameters into a single configuration/builder object (e.g., PosixSysTerminalConfig with fields provider, name/type, pty, encoding/inputEncoding/outputEncoding, nativeSignals, signalHandler) and provide a new constructor overload that accepts this config (or a builder) and delegates to the existing logic. Update usages to construct the config via a fluent builder and pass it in; inside the current constructor implementation move parameter accesses to config.<field> accesses, keeping the existing signal registration loop that calls doRegisterDefaultSignal/doRegisterSignal and raise(signal) unchanged so provider-aware routing remains intact. Ensure backward compatibility by keeping the original constructor signature temporarily and delegating it to the new config-based constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`:
- Around line 231-243: The code currently allocates oldAct and newAct with
Arena.global() inside register(), causing allocations to persist for the JVM
lifetime; change allocation to use a confined arena (e.g., Arena.openConfined()
/ a non-global MemorySession) for the signal structs, attach that arena to the
returned Registration (store it as a field on Registration created in register()
), and ensure the arena is closed when the Registration is unregistered/disposed
(add closing logic in Registration.close/unregister method) so the memory is
reclaimed with the Registration lifecycle; update references in register()
(oldAct, newAct, and returned new Registration(signum, oldAct)) and Registration
class to manage the arena.
In `@terminal/src/main/java/org/jline/terminal/impl/PosixSysTerminal.java`:
- Around line 92-122: The PosixSysTerminal constructor has grown to 9 parameters
making it hard to maintain; extract related parameters into a single
configuration/builder object (e.g., PosixSysTerminalConfig with fields provider,
name/type, pty, encoding/inputEncoding/outputEncoding, nativeSignals,
signalHandler) and provide a new constructor overload that accepts this config
(or a builder) and delegates to the existing logic. Update usages to construct
the config via a fluent builder and pass it in; inside the current constructor
implementation move parameter accesses to config.<field> accesses, keeping the
existing signal registration loop that calls
doRegisterDefaultSignal/doRegisterSignal and raise(signal) unchanged so
provider-aware routing remains intact. Ensure backward compatibility by keeping
the original constructor signature temporarily and delegating it to the new
config-based constructor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1b14e049-97fc-48b4-b30b-d5d98ad64d82
📒 Files selected for processing (6)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.javaterminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmTerminalProvider.javaterminal-jni/src/main/java/org/jline/terminal/impl/jni/JniTerminalProvider.javaterminal/src/main/java/org/jline/terminal/impl/PosixSysTerminal.javaterminal/src/main/java/org/jline/terminal/impl/exec/ExecTerminalProvider.javaterminal/src/main/java/org/jline/terminal/spi/TerminalProvider.java
- Split multi-variable declarations onto separate lines (S1659) - Extract duplicated struct field name strings into constants (S1192) - Replace string concatenation in logger calls with format specifiers (S2629) - Extract dispatchSignal() method to reduce cognitive complexity (S3776) - Narrow catch(Throwable) to catch(Exception | LinkageError) in static init (S1181)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`:
- Around line 239-257: The current code writes to handlers before calling
sigaction, which can leave no Runnable if sigaction fails; change the flow in
the registration logic (the block using sigaction_mh, sa_handler_vh, upcallStub
and returning new Registration) to preserve the existing handler until sigaction
succeeds: do not call handlers.put(signum, handler) before invoking
sigaction_mh; instead allocate newAct/oldAct, set sa_handler_vh, invoke
sigaction_mh, check res, and only on res==0 add handlers.put(signum, handler)
and return new Registration(signum, oldAct); on failure leave handlers unchanged
(or restore the previous entry if you temporarily replaced it) and avoid
removing the prior handler. Apply the same change pattern to the other
registration sites noted (the blocks around lines 277-294 and 309-319).
- Line 211: Change allocations so oldAct/newAct come from a per-registration
arena and ensure that arena is closed on all paths: allocate newAct and oldAct
from reg.arena() (not Arena.global()) where they are created in register(), and
in unregister() call reg.arena().close() after restoring the old handler; also
close reg.arena() in both exception/early-return paths around the sigaction()
calls so the per-registration arena (and the stored oldAction MemorySegment) is
reclaimed even if sigaction() fails.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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
Run ID: 6bcb747d-c456-4b62-b341-df276314c7bd
📒 Files selected for processing (1)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Show resolved
Hide resolved
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Outdated
Show resolved
Hide resolved
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Outdated
Show resolved
Hide resolved
- Document async-signal-safety caveat for upcall stub usage - Use Arena.ofShared() instead of Arena.global() for per-registration allocations to allow memory reclamation on unregister - Only modify handlers map after sigaction() succeeds to preserve existing handler on failure - Close arena in unregister() and on all error paths - Extract "Exception details" string constant (S1192)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java (1)
35-39:⚠️ Potential issue | 🔴 CriticalThe
sigaction()callback still re-enters the JVM through an unsupported FFM path.The new caveat only documents the risk; it does not change the behavior.
sigaction()is still given aLinker.upcallStub(...)pointer, andsignalReceived()still runs on that async signal path. If FFM upcalls are not async-signal-safe, this remains a JVM-crash/deadlock risk rather than a documentation issue.For Java 22+ / OpenJDK Foreign Function & Memory API, is Linker.upcallStub(...) documented or supported as safe to invoke from an asynchronous POSIX signal handler installed via sigaction()?Also applies to: 166-173, 247-248, 336-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java` around lines 35 - 39, The current implementation registers a Linker.upcallStub as the native sigaction handler and still invokes signalReceived(int) on that async signal path, which is unsafe if FFM upcalls are not async-signal-safe; change the design in FfmSignalHandler to stop passing a JVM upcall pointer to sigaction. Instead, register a truly async-signal-safe native handler (a small C trampoline or JVM-provided safe handler) that performs only safe operations (e.g., write(2) to a pre-created self-pipe or set a POSIX sig_atomic_t flag) and return; then have the Java daemon dispatcher thread monitor that pipe or flag and call signalReceived(int) on the Java side. Locate code that creates the upcallStub and installs it via sigaction (symbols: Linker.upcallStub, sigaction installation code, and method signalReceived in class FfmSignalHandler) and replace the upcall approach with the self-pipe/trampoline+dispatcher pattern so no FFM upcall executes directly from the async signal context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`:
- Around line 214-215: The Registration currently only stores the native
MemorySegment oldAction so unregister() (and registerDefault()) can restore
sigaction, but if oldAction.sa_handler already pointed to our upcallStub we must
also restore the previously-registered Java Runnable; add a helper method (e.g.,
isUpcallStub(MemorySegment action)) to detect if oldAction.sa_handler equals
upcallStub and, when true, save and restore the prior Java handler mapping
instead of blindly overwriting handlers with null. Update Registration usage
sites and unregister()/registerDefault() to consult isUpcallStub(oldAction) and
preserve/restore the Java Runnable entry in handlers when the prior native
handler was the shared upcallStub. Ensure the helper is accessible to the code
that creates Registration and to unregister()/registerDefault().
- Around line 241-243: The dispatcher thread is started unconditionally by
ensureDispatcherStarted(), causing a permanent 1ms poller even if native handler
installation fails; modify the flow so ensureDispatcherStarted() is only invoked
after a successful native install (e.g. after
installNativeHandler()/Arena.ofShared() succeeds) and add lifecycle management
to the dispatcher: track active registrations and when the last registration is
removed stop/reset dispatcherThread (and its loop/timer) so it doesn't continue
polling; update methods that remove registrations (the registration removal code
paths around dispatcherThread and any unregister/removeRegistration methods) to
call a shutdown/stop routine on the dispatcher when the registration count
reaches zero, and ensure dispatcherThread is created lazily and marked
null/replaceable so failed or short-lived installs do not leave a permanent
runner.
---
Duplicate comments:
In
`@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`:
- Around line 35-39: The current implementation registers a Linker.upcallStub as
the native sigaction handler and still invokes signalReceived(int) on that async
signal path, which is unsafe if FFM upcalls are not async-signal-safe; change
the design in FfmSignalHandler to stop passing a JVM upcall pointer to
sigaction. Instead, register a truly async-signal-safe native handler (a small C
trampoline or JVM-provided safe handler) that performs only safe operations
(e.g., write(2) to a pre-created self-pipe or set a POSIX sig_atomic_t flag) and
return; then have the Java daemon dispatcher thread monitor that pipe or flag
and call signalReceived(int) on the Java side. Locate code that creates the
upcallStub and installs it via sigaction (symbols: Linker.upcallStub, sigaction
installation code, and method signalReceived in class FfmSignalHandler) and
replace the upcall approach with the self-pipe/trampoline+dispatcher pattern so
no FFM upcall executes directly from the async signal context.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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
Run ID: 023b3576-2a9b-40bd-a1ef-8e775309e59c
📒 Files selected for processing (1)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Outdated
Show resolved
Hide resolved
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Outdated
Show resolved
Hide resolved
- Move ensureDispatcherStarted() after successful sigaction() so the polling thread is not started when native installation fails - Add stopDispatcherIfIdle() to stop the dispatcher when the last handler is removed, preventing a permanent 1ms poller - Save previous Java handler in Registration when oldAction points to our upcall stub; restore it on unregister instead of blindly removing - Add isOurUpcallStub() helper to detect if a sigaction struct's sa_handler points to our shared upcall stub - Fix log message corrupted by earlier rename
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java (1)
336-340: Signal coalescing is acceptable but worth documenting.
pendingSignals.set(signum, 1)(notcompareAndSet) means rapid successive signals of the same type will coalesce into a single dispatch. This is standard signal behavior (POSIX signals don't queue by default), but a brief comment would clarify intent.static void signalReceived(int signum) { if (signum >= 0 && signum < pendingSignals.length()) { + // Multiple rapid signals coalesce (standard POSIX behavior) pendingSignals.set(signum, 1); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java` around lines 336 - 340, The signalReceived method in FfmSignalHandler currently uses pendingSignals.set(signum, 1) which intentionally coalesces rapid identical signals rather than queuing them; add a concise comment above or inside signalReceived (referencing signalReceived and pendingSignals) stating that coalescing is intentional and matches POSIX non-queued signal semantics, so repeated signals of the same type will be collapsed into one pending flag rather than enqueued, to clarify the design decision for future readers and maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`:
- Around line 250-257: Race: after sigaction_mh.invoke succeeds but before
handlers.put(signum, handler) a signal can be lost; fix by inserting the handler
into the handlers map before calling sigaction_mh.invoke (or insert a
placeholder entry) and if sigaction_mh.invoke fails, remove that entry and close
arena/return null; update the Registration construction to use the
already-inserted handler and ensure dispatchSignal()/signalReceived() will find
the handler during the native activation window so no pending signal is dropped.
---
Nitpick comments:
In
`@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`:
- Around line 336-340: The signalReceived method in FfmSignalHandler currently
uses pendingSignals.set(signum, 1) which intentionally coalesces rapid identical
signals rather than queuing them; add a concise comment above or inside
signalReceived (referencing signalReceived and pendingSignals) stating that
coalescing is intentional and matches POSIX non-queued signal semantics, so
repeated signals of the same type will be collapsed into one pending flag rather
than enqueued, to clarify the design decision for future readers and
maintainers.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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
Run ID: 74bc99c7-0e98-482f-84aa-22ea3a9ddd9a
📒 Files selected for processing (2)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.javaterminal/src/main/java/org/jline/terminal/impl/PosixSysTerminal.java
🚧 Files skipped from review as they are similar to previous changes (1)
- terminal/src/main/java/org/jline/terminal/impl/PosixSysTerminal.java
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Outdated
Show resolved
Hide resolved
- Put handler in map before sigaction() to prevent signal loss in the window between native install and handlers.put(); restore previous handler on failure via restoreHandler() helper - Reword comment to avoid SonarCloud S125 false positive - Add signal coalescing comment in signalReceived()
Member
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @gnodet. The following files were modified: * `terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java` * `terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmTerminalProvider.java` * `terminal-jni/src/main/java/org/jline/terminal/impl/jni/JniTerminalProvider.java` * `terminal/src/main/java/org/jline/terminal/impl/PosixSysTerminal.java` * `terminal/src/main/java/org/jline/terminal/impl/exec/ExecTerminalProvider.java` * `terminal/src/main/java/org/jline/terminal/spi/TerminalProvider.java`
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Autofix skipped. No unresolved CodeRabbit review comments with fix instructions found. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java (1)
166-173:⚠️ Potential issue | 🔴 Critical
upcallStub()still needs a documented async-signal-safety story.The JDK material I found describes upcalls as ordinary foreign callbacks (for example, passing a comparator to
qsort), still classifiesupcallStubas a restricted/unsafe operation, and does not document any async-signal-safety guarantee; separately, POSIX signal-handler guidance requires work done in handler context to stay async-signal-safe. That makessigaction() -> upcallStub -> Javatoo risky to rely on here unless there is an explicit JDK guarantee you can point to. (docs.oracle.com)
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`:
- Around line 362-367: Remove the extra blank lines around
ensureDispatcherStarted() and signalNumber(), and fix the misindented Javadoc
before dispatchSignal() so the comment aligns with method declaration; run mvn
spotless:apply to reformat the file with the Palantir Java Format enforced by
Spotless, then re-check the affected regions around ensureDispatcherStarted(),
signalNumber(), and dispatchSignal() (also update the similar
spacing/indentation at the other referenced blocks).
- Around line 326-338: When restoring an earlier native FFM handler in the
sigaction restore path, repopulating handlers via handlers.put(reg.signum(),
reg.previousHandler()) can leave the dispatcher stopped (dispatcherThread ==
null) so pendingSignals won't be drained; update the restore branch in
FfmSignalHandler (around the sigaction_mh.invoke /
isOurUpcallStub(reg.oldAction()) logic) to, after putting the handler back,
ensure the dispatcher is running (restart or create the dispatcher thread if
dispatcherThread is null) instead of only calling stopDispatcherIfIdle, so
pendingSignals will be processed; use the same dispatcher start/initialization
routine used elsewhere (the counterpart to stopDispatcherIfIdle) to avoid
duplicating startup logic.
- Around line 247-271: After the native sigaction install succeeds, exceptions
later (e.g., in ensureDispatcherStarted or isOurUpcallStub) must restore the
process signal disposition to the saved oldAct; modify the catch block in
FfmSignalHandler where sigaction_mh.invoke(signum, newAct, oldAct) is used so it
calls sigaction_mh.invoke(signum, oldAct, MemorySegment.NULL) (or an equivalent
restore path) before closing the Arena and returning null, in addition to
calling restoreHandler(signum, previousHandler), so the native handler is reset
if registration fails after the native install; ensure you reference and use the
existing oldAct MemorySegment and arena (and do the restore before
arena.close()) and leave the Registration(signum, arena, oldAct, ...) creation
unchanged when no exception occurs.
🪄 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
Run ID: 1a340cd3-222b-4576-b6c8-d12b3b0ad7b7
📒 Files selected for processing (6)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.javaterminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmTerminalProvider.javaterminal-jni/src/main/java/org/jline/terminal/impl/jni/JniTerminalProvider.javaterminal/src/main/java/org/jline/terminal/impl/PosixSysTerminal.javaterminal/src/main/java/org/jline/terminal/impl/exec/ExecTerminalProvider.javaterminal/src/main/java/org/jline/terminal/spi/TerminalProvider.java
✅ Files skipped from review due to trivial changes (1)
- terminal-jni/src/main/java/org/jline/terminal/impl/jni/JniTerminalProvider.java
🚧 Files skipped from review as they are similar to previous changes (2)
- terminal/src/main/java/org/jline/terminal/impl/exec/ExecTerminalProvider.java
- terminal/src/main/java/org/jline/terminal/impl/PosixSysTerminal.java
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Show resolved
Hide resolved
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Outdated
Show resolved
Hide resolved
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Show resolved
Hide resolved
…r on post-install failure - In unregister(), call ensureDispatcherStarted() after restoring a previousHandler when the native disposition is our upcall stub, since the dispatcher may have been stopped - In register(), restore the native signal disposition via sigaction() if post-install setup (ensureDispatcherStarted/isOurUpcallStub) fails after the native handler was already installed
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java (1)
166-173:⚠️ Potential issue | 🟠 Major
upcallStubas asigactionhandler remains an unsupported safety assumption.This still relies on
Linker.upcallStub(...)running safely in asynchronous signal context, which is not guaranteed by FFM docs. The caveat in Line 35 acknowledges this, but runtime behavior can still be JVM-dependent and fragile.For Java 22+ Foreign Function & Memory API, is Linker.upcallStub documented as async-signal-safe when used as a POSIX sigaction handler, and are there official examples or guarantees for this usage?Also applies to: 35-39
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java` around lines 166 - 173, The code currently uses Linker.upcallStub (the stub variable created in the static init) as the POSIX sigaction handler which is not guaranteed async-signal-safe; replace this approach by removing the upcallStub usage in FfmSignalHandler and instead install a native (C) sigaction trampoline that performs only async-signal-safe operations (e.g., write the signum to a pipe or eventfd) and expose a safe Java-visible native initializer (e.g., installSignalHandler) that the class calls during startup; have a dedicated Java poll thread read that pipe/eventfd and invoke FfmSignalHandler.signalReceived(signum) from normal Java context (not from signal handler). Ensure you reference and change the locations using upcallStub, linker, the stub variable, and the signalReceived method so signal delivery is routed through the native trampoline + pipe and not via Linker.upcallStub.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`:
- Around line 276-281: The catch block in register() restores native handlers
and closes arena but doesn't reconcile the dispatcher lifecycle or the handlers
map, risking inconsistency between handlers and dispatcher state; update the
catch path to also stop/unregister the dispatcher and remove any
partially-registered entry from the handlers map (or roll back any insertion) so
dispatcher state matches handlers, referencing register(),
restoreHandler(signum, previousHandler), arena.close(), handlers and the
dispatcher instance so any created dispatcher is properly shutdown/removed on
failure.
---
Duplicate comments:
In
`@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`:
- Around line 166-173: The code currently uses Linker.upcallStub (the stub
variable created in the static init) as the POSIX sigaction handler which is not
guaranteed async-signal-safe; replace this approach by removing the upcallStub
usage in FfmSignalHandler and instead install a native (C) sigaction trampoline
that performs only async-signal-safe operations (e.g., write the signum to a
pipe or eventfd) and expose a safe Java-visible native initializer (e.g.,
installSignalHandler) that the class calls during startup; have a dedicated Java
poll thread read that pipe/eventfd and invoke
FfmSignalHandler.signalReceived(signum) from normal Java context (not from
signal handler). Ensure you reference and change the locations using upcallStub,
linker, the stub variable, and the signalReceived method so signal delivery is
routed through the native trampoline + pipe and not via Linker.upcallStub.
🪄 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
Run ID: b1bd6b2e-bf7d-4d01-b3e3-d56f68b7c3a6
📒 Files selected for processing (1)
terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java
Comment on lines
+276
to
+281
| } catch (Throwable t) { | ||
| logger.log(Level.FINE, "Error registering FFM signal handler for {0}", name); | ||
| logger.log(Level.FINE, EXCEPTION_DETAILS, t); | ||
| restoreHandler(signum, previousHandler); | ||
| arena.close(); | ||
| return null; |
There was a problem hiding this comment.
Reconcile dispatcher lifecycle in the register() failure path.
If post-install logic throws, this path restores handlers/native disposition but does not re-evaluate dispatcher state. That can leave dispatcher state inconsistent with handlers contents in rare failure cases.
Suggested adjustment
} catch (Throwable t) {
logger.log(Level.FINE, "Error registering FFM signal handler for {0}", name);
logger.log(Level.FINE, EXCEPTION_DETAILS, t);
restoreHandler(signum, previousHandler);
+ if (previousHandler != null) {
+ ensureDispatcherStarted();
+ } else {
+ stopDispatcherIfIdle();
+ }
arena.close();
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@terminal-ffm/src/main/java/org/jline/terminal/impl/ffm/FfmSignalHandler.java`
around lines 276 - 281, The catch block in register() restores native handlers
and closes arena but doesn't reconcile the dispatcher lifecycle or the handlers
map, risking inconsistency between handlers and dispatcher state; update the
catch path to also stop/unregister the dispatcher and remove any
partially-registered entry from the handlers map (or roll back any insertion) so
dispatcher state matches handlers, referencing register(),
restoreHandler(signum, previousHandler), arena.close(), handlers and the
dispatcher instance so any created dispatcher is properly shutdown/removed on
failure.
Use a nativeInstalled flag and extract bestEffortRestore() helper to avoid nested try-catch. Suppresses S1181 on the helper since MethodHandle.invoke declares throws Throwable.
|
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
1 participant
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
TerminalProvidercan use platform-specific mechanisms instead of relying onsun.misc.SignalreflectionFfmTerminalProvidernow installs signal handlers via nativesigaction()withSA_RESTART, eliminating reflection and internal API usage when the FFM provider is active (Java 22+)sun.misc.Signalthrough the default implementation — no behavioral changeKey changes
TerminalProvider: new default methodsregisterSignal()/registerDefaultSignal()/unregisterSignal()delegating toSignals.javaFfmSignalHandler(new): self-contained FFM signal handler with platform-specificstruct sigactionlayouts (macOS, Linux, FreeBSD),SA_RESTARTflag, shared upcall stub, and atomic-flag dispatcher thread for signal safetyFfmTerminalProvider: overrides signal methods to useFfmSignalHandlerwith automatic fallback toSignals.javaPosixSysTerminal: accepts optionalTerminalProvider, delegates signal registration through itFfmTerminalProvider,JniTerminalProvider,ExecTerminalProvider) passthistoPosixSysTerminalNon-breaking guarantees
Terminal.handle(Signal, SignalHandler)public API is unchangedCloses #1747
Test plan
Summary by CodeRabbit
New Features
Improvements