Fix #1575: Enforce terminal closure with backward compatibility (v3) by gnodet · Pull Request #1577 · jline/jline3

gnodet · 2026-02-05T07:38:08Z

Summary

Fixes #1575 by implementing terminal closure enforcement in JLine 3.x with backward compatibility by default. After a terminal is closed, accessing its streams will log warnings (soft mode) or throw exceptions (strict mode via system property).

Problem

As reported in #1575, terminals could still be used after calling close(), leading to unexpected behavior:

Terminal terminal = TerminalBuilder.terminal();
terminal.writer().println("foo");
terminal.close();

// This should fail but didn't:
terminal.writer().println("still works!"); // ❌ Should throw

Solution

Implemented two-level closure enforcement with backward compatibility:

1. Terminal-Level Checks

Added closed flag to AbstractTerminal
Added checkClosed() method that throws IllegalStateException
All terminal accessor methods now check if terminal is closed
Prevents accessing terminal methods after closure

2. Stream-Level Checks (Backward Compatible)

Added closed flag and checkClosed() to NonBlockingInputStream and NonBlockingReader
Default behavior (soft mode): Logs a warning when accessing closed streams
Opt-in strict mode: Set -Djline.terminal.strictClose=true to throw ClosedException
Prevents using held stream references after terminal closure

Behavior in JLine 3.x (This PR)

Soft mode by default (backward compatible) - accessing closed streams logs warnings:

Terminal terminal = TerminalBuilder.terminal();
PrintWriter writer = terminal.writer();
terminal.close();

terminal.writer().println("test");  // ✓ Throws IllegalStateException
writer.println("test");              // ⚠️ Logs warning (default)

Opt-in strict mode - set -Djline.terminal.strictClose=true to throw exceptions:

// With -Djline.terminal.strictClose=true:
writer.println("test");  // ✓ Throws ClosedException

Changes

System Property Constant

TerminalBuilder.java

Added PROP_STRICT_CLOSE = "jline.terminal.strictClose" constant
Centralized system property name in main API
Follows existing pattern (all PROP_* constants in TerminalBuilder)

Core Implementation

AbstractTerminal.java

Added protected volatile boolean closed field
Added checkClosed() method
Set closed = true in doClose()

Terminal Implementations (6 files)

PosixSysTerminal.java - Added checks to 4 methods
AbstractWindowsTerminal.java - Added check to 1 method
PosixPtyTerminal.java - Added checks to 4 methods
AbstractPosixTerminal.java - Added check to 1 method
DumbTerminal.java - Added checks to 8 methods
LineDisciplineTerminal.java - Added checks to 8 methods

NonBlocking Stream Classes

NonBlockingInputStream.java - Added closed flag, checkClosed() with STRICT_CLOSE logic, uses PROP_STRICT_CLOSE constant
NonBlockingReader.java - Added closed flag, checkClosed() with STRICT_CLOSE logic, uses PROP_STRICT_CLOSE constant
Both classes implement soft mode (warning) by default, strict mode (exception) when property is set

Tests

StrictCloseTest.java (new)

Tests strict mode behavior (requires -Djline.terminal.strictClose=true)
Verifies that held stream references throw ClosedException in strict mode
Tests are disabled by default (require system property to run)

Code Quality Improvements

As part of this fix, also refactored repeated code:

Centralized PROP_STRICT_CLOSE constant in TerminalBuilder
Replaced 22 inline if (closed) checks with calls to checkClosed()
Net reduction of 23 lines of code

Statistics

10 files changed (excluding other unrelated changes in the branch)
57 insertions(+) for the refactoring
80 deletions(-) for the refactoring

Backward Compatibility

✅ JLine 3.x: This maintains backward compatibility by default.

Default behavior (soft mode):

Accessing closed terminals through terminal methods: Throws IllegalStateException ✓
Accessing held stream references: Logs warning ⚠️

Opt-in strict mode (-Djline.terminal.strictClose=true):

Accessing closed terminals: Throws IllegalStateException ✓
Accessing held stream references: Throws ClosedException ✓

Migration recommendation:

Use default soft mode in JLine 3.x to identify issues via warnings
Fix code to properly manage terminal lifecycle
Optionally enable strict mode to enforce proper usage
Upgrade to JLine 4.x when ready (strict mode by default)

Testing

Run strict mode tests:

mvn test -Dtest=StrictCloseTest -Djline.terminal.strictClose=true

…s (JLine 3.x) - Add closed state checking to NonBlockingReader and NonBlockingInputStream base classes - Soft mode is default (backward compatible): logs WARNING instead of throwing exception - Strict mode can be enabled with -Djline.terminal.strictClose=true - Update all concrete implementations to call checkClosed() in read methods - Remove ClosedChecking* wrapper classes (no longer needed) - Update terminal implementations to use base classes directly - Add StrictCloseTest with tests marked as @disabled (strict mode not default in 3.x) - HeldStreamReferenceTest tests soft mode behavior (default in 3.x)

…OSE constant - Added PROP_STRICT_CLOSE constant to TerminalBuilder for system property name - Updated NonBlockingInputStream and NonBlockingReader to use the constant - Added checkClosed() method to AbstractTerminal for consistent error handling - Replaced inline closed checks in Terminal implementations with checkClosed() calls - Reduces code duplication and improves maintainability

augmentcode · 2026-02-05T07:46:14Z

🤖 Augment PR Summary

Summary: This PR addresses #1575 by enforcing terminal closure while keeping JLine 3.x backward-compatible by default.

Changes:

Adds TerminalBuilder.PROP_STRICT_CLOSE to centralize the jline.terminal.strictClose property.
Introduces a closed flag and checkClosed() in AbstractTerminal, and wires terminal implementations to reject access after close().
Adds close-awareness to NonBlockingInputStream and NonBlockingReader with two modes: soft (log a warning) vs strict (throw ClosedException).
Updates non-blocking implementations to invoke checkClosed() during reads and to ensure the base closed flag is set on close.
Adjusts terminal close behavior in a few implementations (e.g., dumb/system terminal stream wrapping) to avoid closing underlying system file descriptors.
Adds new tests covering held-stream-reference behavior in soft mode and a disabled test class demonstrating strict mode expectations.

Technical Notes: Strict mode is opt-in via -Djline.terminal.strictClose=true; default remains warning-only to ease migration.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. 5 suggestions posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-02-05T07:46:15Z

    }

    protected void doClose() throws IOException {
+        closed = true;


Because closed is set before status.close(), any Status cleanup that writes via terminal.puts()/writer() can now throw IllegalStateException (many writer()/output()/... methods call checkClosed()), causing Terminal.close() to fail partway through. This can leave resources/hooks unclosed when getStatus() was used.

Severity: high

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

✅ RESOLVED - This issue has been fixed. The closed = true assignment has been moved to AFTER status.close() (line 176 in AbstractTerminal.java). Additionally, we removed checkClosed() calls from getter methods like writer() and reader(), so status.close() can safely call terminal.puts()/writer() during cleanup without throwing exceptions.

augmentcode · 2026-02-05T07:46:15Z

     * @throws IOException if anything wrong happens
     */
    public synchronized int read(long timeout, boolean isPeek) throws IOException {
+        checkClosed();


read() now calls checkClosed(), but this class’s close() override never sets NonBlockingInputStream.closed, so checkClosed() won’t ever detect closure (and strict/soft close behavior won’t engage). This seems likely to make -Djline.terminal.strictClose ineffective for the most common input stream implementation.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

✅ RESOLVED - This issue has been fixed. NonBlockingInputStreamImpl.close() now calls super.close() at line 78, which properly sets the closed flag in the parent NonBlockingInputStream class. The checkClosed() method will now correctly detect closure and enforce strict/soft close behavior.

augmentcode · 2026-02-05T07:46:15Z

     *   read timed out.
     */
    protected synchronized int read(long timeout, boolean isPeek) throws IOException {
+        checkClosed();


read() now calls checkClosed(), but NonBlockingReaderImpl.close() doesn’t set NonBlockingReader.closed, so the new strict/soft close behavior may never trigger for this implementation. That can also make the warning/ClosedException behavior inconsistent across NonBlockingReader implementations.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

✅ RESOLVED - This issue has been fixed. NonBlockingReaderImpl.close() now calls super.close() at line 81, which properly sets the closed flag in the parent NonBlockingReader class. The checkClosed() method will now correctly detect closure and enforce strict/soft close behavior consistently across all NonBlockingReader implementations.

augmentcode · 2026-02-05T07:46:15Z

                needOpt = null;
            } else if (!arg.startsWith("-")
-                    || (arg.length() > 1 && Character.isDigit(arg.charAt(1)))
+                    || (arg.length() > 2 && Character.isDigit(arg.charAt(1)) && Character.isDigit(arg.charAt(2)))


The new numeric-argument heuristic only treats -<digit><digit>... as a non-option; negative numbers like -1.5 or -1e3 would now be parsed as options, which may break existing callers passing numeric args. If that’s not intended, the digit check may need to account for additional numeric formats.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

❌ NOT PART OF THIS PR - The file builtins/src/main/java/org/jline/builtins/Options.java is not modified in this PR. This PR focuses on terminal closure behavior and the jline.terminal.strictClose property. This comment appears to be related to a different change or may have been flagged incorrectly.

augmentcode · 2026-02-05T07:46:15Z

+        LineReader reader1 = LineReaderBuilder.builder().build();
+        assertFalse(reader1.isSet(LineReader.Option.DISABLE_EVENT_EXPANSION));
+
+        System.setProperty("org.jline.reader.props.disable-event-expansion", "on");


This test clears org.jline.reader.props.disable-event-expansion in finally, which can erase a pre-existing value set by the build environment and introduce cross-test interference. It may be safer to capture and restore the previous value instead of always clearing.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

❌ NOT PART OF THIS PR - The file reader/src/test/java/org/jline/reader/impl/SystemOptionsTest.java is not modified in this PR. This PR focuses on terminal closure behavior and the jline.terminal.strictClose property. This comment appears to be related to a different change or may have been flagged incorrectly.

…m close() methods - Move 'closed = true' to after status.close() in AbstractTerminal.doClose() to prevent IllegalStateException during cleanup - Add super.close() calls in NonBlockingInputStreamImpl and NonBlockingReaderImpl to properly set the closed flag for strict/soft close behavior - 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 NonBlockingInputStream and NonBlockingReader subclasses for consistency: - NonBlockingPumpInputStream.readBuffered() - 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.

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 improves testability and allows different stream instances to have different strictClose settings based on when they were created. Benefits: - Tests can verify both strict and soft close mode behavior - Different stream instances can have different strictClose settings - More flexible and testable design

gnodet · 2026-02-06T10:59:58Z

augment review

augmentcode

Review completed. 5 suggestions posted.

Comment augment review to trigger a new review at any time.

augmentcode · 2026-02-06T11:05:03Z

+     * System property to control strict close mode for terminals and streams.
+     * <p>
+     * In JLine 3.x, soft mode is enabled by default for backward compatibility.
+     * When set to "true", accessing closed terminals or streams will throw a ClosedException.


The Javadoc says strict close makes "closed terminals" throw ClosedException, but terminal-level checks in AbstractTerminal.checkClosed() throw IllegalStateException regardless of PROP_STRICT_CLOSE (the property only affects NonBlocking* streams). Consider aligning this doc with the actual exception/behavior.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

augmentcode · 2026-02-06T11:05:03Z


        @Override
        public int read(long timeout, boolean isPeek) throws IOException {
+            checkClosed();


checkClosed() is now called, but close() in this class doesn't call super.close(), so the closed flag used by checkClosed() is never set and strict/soft-close behavior won't trigger here. This makes strict-close enforcement/warnings inconsistent for NonBlockingReaderInputStream/NonBlockingInputStreamReader.

Severity: medium

Other Locations

terminal/src/main/java/org/jline/utils/NonBlocking.java:182

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

augmentcode · 2026-02-06T11:05:03Z


    @Override
    public synchronized int read(long timeout, boolean isPeek) throws IOException {
+        checkClosed();


This class has its own closed field/close() implementation and does not update the inherited NonBlockingInputStream.closed, so the newly-added checkClosed() call won’t ever observe closure (and strict/soft-close behavior won’t apply). This also creates two independent "closed" states (wait() uses the local one, checkClosed() uses the inherited one).

Severity: medium

Other Locations

terminal/src/main/java/org/jline/utils/NonBlockingPumpInputStream.java:108

terminal/src/main/java/org/jline/utils/NonBlockingPumpReader.java:71

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

augmentcode · 2026-02-06T11:05:03Z

+ * In JLine 3.x, the default behavior is "soft close": accessing streams after terminal closure
+ * logs a WARNING but does not throw an exception, preserving backward compatibility.
+ * When the system property {@code jline.terminal.strictClose=true} is set, accessing closed
+ * terminal streams throws {@code IllegalStateException}.


This Javadoc says strict close makes accessing closed terminal streams throw IllegalStateException, but stream-level strict-close appears to throw ClosedException (and terminal-level access throws IllegalStateException regardless of the property). It may be worth clarifying which APIs throw which exception in strict mode.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

augmentcode · 2026-02-06T11:05:03Z

+    }
+
+    /**
+     * Demonstrates the enhanced behavior: held references also throw after terminal close.


This demonstration claims held references "should now throw" IllegalStateException, but the implemented behavior is soft-close warnings by default and ClosedException for strict mode (and PrintWriter typically won’t throw on writes even when closed). As-is, running this demo would likely misrepresent the new close semantics.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

- Fix TerminalBuilder javadoc to clarify terminal-level vs stream-level exceptions - Fix NonBlocking wrapper classes to call super.close() so checkClosed() works properly - Fix NonBlockingPumpInputStream and NonBlockingPumpReader to call super.close() to update inherited closed field - Update HeldStreamReferenceTest javadoc to clarify exception types These changes improve documentation accuracy and ensure the checkClosed() mechanism works correctly across all NonBlocking implementations.

…iplineTerminal - Removed slaveInputPump field which was redundant with slaveInput - Changed slaveInput type from NonBlockingInputStream to NonBlockingPumpInputStream - Removed empty try block in doClose() This simplifies the code while maintaining the same functionality.

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. This brings PosixSysTerminal in line with PosixPtyTerminal and DumbTerminal which already use this simplified pattern.

Updated documentation to match v4 with comprehensive explanation of: - 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 - 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.

gnodet added 3 commits February 3, 2026 18:17

Add soft terminal closure checking with warnings (JLine 3.x)

b6e76b4

gnodet changed the title ~~Refactor: Extract repeated closed check and centralize PROP_STRICT_CLOSE constant (v3)~~ Fix #1575: Enforce terminal closure with backward compatibility (v3) Feb 5, 2026

gnodet mentioned this pull request Feb 5, 2026

Fix #1575: Enforce terminal closure - prevent use-after-close bugs (v4) #1576

Merged

augmentcode Bot reviewed Feb 5, 2026

View reviewed changes

gnodet changed the base branch from master to jline-3.x February 5, 2026 07:57

gnodet added 4 commits February 5, 2026 10:34

augmentcode Bot reviewed Feb 6, 2026

View reviewed changes

gnodet added 6 commits February 6, 2026 14:37

Remove deprecated PROP_STRICT_CLOSE constant

f595eef

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.

gnodet added this to the 3.30.7 milestone Feb 10, 2026

gnodet merged commit 1189e96 into jline:jline-3.x Feb 10, 2026
9 checks passed

gnodet deleted the fix-1575-soft-close-v3 branch March 3, 2026 12:36

Conversation

gnodet commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Solution

1. Terminal-Level Checks

2. Stream-Level Checks (Backward Compatible)

Behavior in JLine 3.x (This PR)

Changes

System Property Constant

Core Implementation

Tests

Code Quality Improvements

Statistics

Backward Compatibility

Testing

Related

Uh oh!

augmentcode Bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

gnodet commented Feb 6, 2026

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

gnodet commented Feb 5, 2026 •

edited

Loading

augmentcode Bot commented Feb 5, 2026 •

edited

Loading