Fix #1575: Enforce terminal closure - prevent use-after-close bugs (v4)#1576
Merged
gnodet merged 21 commits intojline:masterfrom Feb 10, 2026
Merged
Fix #1575: Enforce terminal closure - prevent use-after-close bugs (v4)#1576gnodet merged 21 commits intojline:masterfrom
gnodet merged 21 commits intojline:masterfrom
Conversation
- Add volatile boolean closed field and checkClosed() method to NonBlockingReader and NonBlockingInputStream base classes - Update all concrete implementations to call checkClosed() in read methods - Remove InputStreamWrapper from PosixPtyTerminal (no longer needed) - Update NonBlockingPumpReader and NonBlockingPumpInputStream to use base class closed field - Ensure all terminal implementations properly close their input streams in doClose() This ensures that after a terminal is closed, any attempt to read from its streams will throw a ClosedException, addressing issue jline#1575.
- Move jline.terminal.strictClose system property handling to NonBlockingReader and NonBlockingInputStream base classes - Update checkClosed() to support soft mode (log warnings) and strict mode (throw exceptions) - Remove ClosedChecking* wrapper classes (ClosedCheckingReader, ClosedCheckingInputStream, ClosedCheckingOutputStream, ClosedCheckingPrintWriter) - Remove all ClosedChecking* wrapper usage from terminal implementations (PosixSysTerminal, LineDisciplineTerminal, DumbTerminal, AbstractWindowsTerminal) - Simplify terminal implementations by using NonBlocking streams directly This consolidates the closed state checking into the base classes where it belongs, as the soft/strict mode is a global behavior controlled by a system property, not terminal-specific. The default soft mode provides backward compatibility by logging warnings instead of throwing exceptions when accessing closed streams.
Revert baseReader back to reader to match the original code
Use slaveOutput directly instead of filteringOutput variable to match original code
- Add StrictCloseTest with tests for strict close mode behavior - Tests are disabled by default and require -Djline.terminal.strictClose=true - Fix HeldStreamReferenceTest javadoc to mention ClosedException instead of IllegalStateException
- Change default value of jline.terminal.strictClose to true (strict mode) - Update javadoc to reflect that strict mode is default in 4.x - Enable StrictCloseTest tests (now run by default) - Disable HeldStreamReferenceTest tests (require -Djline.terminal.strictClose=false) - Users can disable strict mode with -Djline.terminal.strictClose=false for backward compatibility
…osed() - Added protected checkClosed() method in AbstractTerminal - Replaced 35 inline 'if (closed) throw IllegalStateException' checks - Updated 7 Terminal implementation classes to use checkClosed() - Improves code maintainability and reduces duplication
🤖 Augment PR SummarySummary: Tightens JLine 4.x terminal lifecycle enforcement to prevent use-after-close by making closed terminals/streams fail fast (with an optional compatibility mode). Changes:
Technical Notes: Soft-close behavior is controlled by 🤖 Was this summary useful? React with 👍 or 👎 |
…oClose() - Move 'closed = true' to after status.close() to prevent IllegalStateException during cleanup operations - NonBlocking stream implementations already have super.close() calls in v4 - Apply code formatting fixes
- Fix DumbTerminal anonymous NonBlockingInputStream to properly close underlying stream This ensures the background thread in NonBlockingInputStreamImpl is shut down when the terminal closes, preventing JVM hangs - Add checkClosed() calls to all NonBlockingInputStream and NonBlockingReader subclasses for consistency: - NonBlockingPumpInputStream.read() and readBuffered() - NonBlockingPumpReader.read() - NonBlockingReaderInputStream.read() (inner class in NonBlocking.java) - NonBlockingInputStreamReader.read() (inner class in NonBlocking.java) This ensures all stream implementations consistently check for closed state before performing operations, providing uniform behavior across the codebase.
Wrap system streams (FileDescriptor.in/out/err) in non-closeable wrappers to prevent closing the underlying file descriptors when the terminal is closed. This ensures that: - System.in/out/err remain usable after closing a system terminal - Multiple system terminals can be created sequentially - Surefire communication channel is not corrupted by closing System.out Fixes SystemOutCloseTest failure on GitHub Actions.
Define PROP_STRICT_CLOSE in TerminalBuilder and use it consistently in NonBlockingInputStream and NonBlockingReader instead of hardcoding the 'jline.terminal.strictClose' string. This improves maintainability and follows the same pattern as v3.
Changed strictClose from a static final field to an instance final field in NonBlockingInputStream and NonBlockingReader. The value is now determined at construction time from the system property, allowing tests to set the property before creating streams. This enables all 6 tests in HeldStreamReferenceTest which were previously disabled. Each test now sets the system property to enable soft close mode before creating the terminal, then restores strict mode after completion. Benefits: - Tests can now verify soft close mode behavior - Different stream instances can have different strictClose settings - More flexible and testable design
The demo was incorrectly catching IllegalStateException for held stream references. Fixed to properly demonstrate the actual behavior: - PrintWriter doesn't throw exceptions - it sets an error flag instead (use checkError() to detect failures) - NonBlockingReader throws ClosedException (an IOException) in strict mode, not IllegalStateException Updated javadoc and comments to reflect the correct behavior. Addresses review comment from augmentcode bot in PR jline#1576.
Member
Author
|
augment review |
- Fix NonBlocking wrapper classes to call super.close() so checkClosed() works properly - Remove empty try block in LineDisciplineTerminal.doClose() - Add try-finally blocks in doClose() methods to ensure all resources are closed even if exceptions occur - Update javadoc for NonBlockingInputStream and NonBlockingReader to clarify strict vs soft mode behavior - Fix HeldStreamReferenceTest to restore previous system property value instead of hardcoding 'true' These changes improve resource cleanup reliability and test isolation.
The slaveInput and slaveInputPipe refer to the same NonBlockingPumpInputStream object. Closing slaveInputPipe already closes the pump, so we don't need to separately close slaveInput. This reverts to the original behavior from master branch.
…close() + reader.shutdown() Since reader.close() delegates to input.close() and close() is idempotent, we can simplify the cleanup logic by just calling reader.close(). This is clearer and more maintainable than manually closing the input and shutting down the reader separately.
Added detailed documentation explaining: - Two levels of closure enforcement (terminal-level vs stream-level) - Property values and their behavior (strict vs soft mode) - Default values for JLine 3.x vs 4.x - Example code demonstrating the difference - Cross-references to related classes
Changed from boolean strictClose property to string-based closeMode with three modes: - strict: Throw ClosedException (default for v4) - warn: Log warning but continue (default for v3) - lenient: Silently allow access (no warning, no exception) Changes: - Renamed PROP_STRICT_CLOSE to PROP_CLOSE_MODE (old constant deprecated) - Property name: jline.terminal.strictClose -> jline.terminal.closeMode - Backward compatibility: old property still works (true->strict, false->warn) - Updated NonBlockingInputStream and NonBlockingReader to use enum-based mode - Updated all tests to use new property name and values - Added comprehensive javadoc explaining all three modes
Since this hasn't been released yet, we can remove the deprecated constant and backward compatibility code entirely. This simplifies the implementation to only use the new PROP_CLOSE_MODE property.
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.
Summary
Fixes #1575 by implementing strict terminal closure enforcement in JLine 4.x. After a terminal is closed, all attempts to access its streams (reader, writer, input, output) will throw exceptions, preventing use-after-close bugs.
Problem
As reported in #1575, terminals could still be used after calling
close(), leading to unexpected behavior:Solution
Implemented two-level closure enforcement:
1. Terminal-Level Checks
closedflag toAbstractTerminalcheckClosed()method that throwsIllegalStateExceptionreader(),writer(),input(),output(),getAttributes(),setAttributes(),getSize(),setSize()) now check if terminal is closed2. Stream-Level Checks
closedflag andcheckClosed()toNonBlockingInputStreamandNonBlockingReaderClosedExceptionwhen accessed after closureBehavior in JLine 4.x (This PR)
Strict mode by default - accessing closed terminals or streams throws exceptions:
Optional soft mode - set
-Djline.terminal.strictClose=falseto log warnings instead of throwing (for backward compatibility during migration).Changes
Core Implementation
AbstractTerminal.java
protected volatile boolean closedfieldcheckClosed()methodclosed = trueindoClose()Terminal Implementations (7 files)
AbstractPosixTerminal.java- Added checks to 4 methodsAbstractWindowsTerminal.java- Added checks to 7 methodsDumbTerminal.java- Added checks to 8 methods + properdoClose()LineDisciplineTerminal.java- Added checks to 8 methods + fixed close orderPosixPtyTerminal.java- Added checks to 4 methods + removed wrapper classPosixSysTerminal.java- Added checks to 4 methods + close input streamAbstractPty.java- Added check toPtyInputStream.read()NonBlocking Stream Classes
NonBlockingInputStream.java- Addedclosedflag,checkClosed(),close()methodNonBlockingReader.java- Addedclosedflag,checkClosed(),close()methodNonBlockingInputStreamImpl.java- CallcheckClosed()inread()NonBlockingReaderImpl.java- CallcheckClosed()inread()NonBlockingPumpInputStream.java- Use base classclosedfieldNonBlockingPumpReader.java- Use base classclosedfieldTests
StrictCloseTest.java (new)
ClosedExceptionin strict mode (default)HeldStreamReferenceTest.java (new)
-Djline.terminal.strictClose=false)StreamClosureDemonstration.java (new)
Code Quality Improvements
As part of this fix, also refactored repeated code:
if (closed)checks with calls tocheckClosed()Statistics
Breaking Changes
Migration path: Set
-Djline.terminal.strictClose=falsetemporarily to identify issues via warnings, then fix the code to properly manage terminal lifecycle.Testing
Run the demonstration:
Run strict mode tests:
mvn test -Dtest=StrictCloseTestRun soft mode tests:
mvn test -Dtest=HeldStreamReferenceTest -Djline.terminal.strictClose=falseRelated