Skip to content

fix: restore waitDirty timeout in forced dump to prevent busy-loop spinning (fixes #1768)#1769

Open
gnodet wants to merge 1 commit intomasterfrom
ci-issue-1768
Open

fix: restore waitDirty timeout in forced dump to prevent busy-loop spinning (fixes #1768)#1769
gnodet wants to merge 1 commit intomasterfrom
ci-issue-1768

Conversation

@gnodet
Copy link
Copy Markdown
Member

@gnodet gnodet commented Apr 7, 2026

Summary

Commit c2a0b89 swapped waitDirty(timeout) || forceDump to forceDump || waitDirty(timeout) in the three dump(timeout, forceDump, ...) overloads. Due to Java's short-circuit evaluation, when forceDump=true, waitDirty(timeout) is never called, so the method returns immediately instead of waiting up to timeout ms. This causes busy-loop spinning for callers that rely on the timeout for throttling redraws.

This restores the original operand order so waitDirty(timeout) is always evaluated first, preserving the throttling behavior.

Fixes #1768

Summary by CodeRabbit

  • Bug Fixes

    • Fixed screen terminal dump operation to respect the specified timeout duration even when force dump is enabled, ensuring the operation blocks for the full timeout period as expected.
  • Tests

    • Added test to verify timeout behavior is correctly honored in force dump scenarios.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 7, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b44b4e90-9210-44ea-818e-6e7927694990

📥 Commits

Reviewing files that changed from the base of the PR and between 28dd626 and 4f59b4a.

📒 Files selected for processing (2)
  • builtins/src/main/java/org/jline/builtins/ScreenTerminal.java
  • builtins/src/test/java/org/jline/builtins/ScreenTerminalTest.java

📝 Walkthrough

Walkthrough

This change reverts a conditional logic inversion in three overloaded dump() methods, restoring evaluation of waitDirty(timeout) before forceDump. The fix ensures that forcing a dump still respects the timeout parameter by waiting before checking the dirty state. A new test validates that forced dumps block for the specified timeout.

Changes

Cohort / File(s) Summary
Bug Fix
builtins/src/main/java/org/jline/builtins/ScreenTerminal.java
Reordered conditional logic in three dump() method overloads from forceDump || waitDirty(timeout) to waitDirty(timeout) || forceDump, ensuring the timeout is respected even when forcing a dump.
Test Addition
builtins/src/test/java/org/jline/builtins/ScreenTerminalTest.java
Added new test method testForceDumpWaitsForTimeout() that verifies dump(timeout, forceDump=true) blocks for at least the specified timeout duration even when the terminal is not dirty.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested labels

bug, java

Poem

🐰 A timeout was lost in the shuffle of true,
Where forceDump rushed in before waiting was through,
But now we restore the original way—
Let patience precede the dump of the day!
The timeout awaits, as it always should do. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: restoring waitDirty timeout order in forced dump to prevent busy-loop spinning, and explicitly references the related issue #1768.
Linked Issues check ✅ Passed The code changes directly address issue #1768 by restoring the correct operand order (waitDirty(timeout) || forceDump) to ensure timeout blocking occurs even when forceDump is true, matching the issue's requirement.
Out of Scope Changes check ✅ Passed All changes are scoped to the stated objective: restoring waitDirty timeout in the three dump method overloads and adding a test to verify the timeout behavior with forced dumps.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci-issue-1768

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 7, 2026

@gnodet gnodet added this to the 4.0.12 milestone Apr 7, 2026
@gnodet gnodet added the bug label Apr 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ScreenTerminal always dumps immediatly if forced

1 participant