Skip to content

Do not share terminal state between concurrent terminals in one process - #44

Merged
alexey-milovidov merged 1 commit into
masterfrom
clickhouse/thread-safe-terminal-state-master
Oct 12, 2026
Merged

alexey-milovidov merged 1 commit into
masterfrom
clickhouse/thread-safe-terminal-state-master

Conversation

@alexey-milovidov

Copy link
Copy Markdown
Member

Several terminals can work concurrently in one process. ClickHouse does this with client sessions embedded into the server (the web terminal and SSH), each attached to its own pseudo-terminal. Some replxx state was process-wide, so these sessions could affect each other:

  • SIGWINCH receiver (_terminal_): every terminal that entered raw mode stored itself in a non-atomic global, and leaving raw mode cleared it even if another terminal had registered. Now only a terminal that installed the window change handler registers itself, the registration is atomic (so the signal handler can read it safely), and a terminal unregisters only itself.
  • Partial UTF-8 sequence in read_unicode_character: concurrent multi-byte input could corrupt each other. Also, a race between the bounds check and the write allowed a one-byte out-of-bounds write. The state is now thread_local.
  • Escape sequence modifiers (thisKeyMetaCtrl): concurrent terminals could get each other's Ctrl/Alt bits. Now thread_local.
  • ansi_color buffer: now thread_local. The result is consumed right away by the calling thread.

The remaining mutable-looking globals are set only during static initialization.

ClickHouse PR: ClickHouse/ClickHouse#125510

ClickHouse currently uses this change on top of the clickhouse/history-recalled-hints-fix branch (commit e8e3d2f). This PR has the same change cherry-picked onto master.

🤖 Generated with Claude Code

Several terminals can work concurrently in one process, e.g. client sessions
embedded into a server, each attached to its own pseudo-terminal.

- Only a terminal that installed the `SIGWINCH` handler registers itself as the
  receiver of window size notifications, the registration is atomic, and a
  terminal unregisters only itself.
- The partially read UTF-8 sequence in `read_unicode_character`, the modifiers
  of the escape sequence being parsed, and the buffer of `ansi_color` are per
  thread. Previously concurrent multi-byte input could corrupt each other, and a
  race allowed a one-byte out-of-bounds write.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@alexey-milovidov alexey-milovidov left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@alexey-milovidov alexey-milovidov self-assigned this Oct 12, 2026
@alexey-milovidov
alexey-milovidov merged commit 1109297 into master Oct 12, 2026
1 check failed
alexey-milovidov added a commit to ClickHouse/ClickHouse that referenced this pull request Oct 12, 2026
`master` of `replxx` now contains the fix of the shared terminal state. The new
commit merges it with the history recall changes used by ClickHouse and fixes
Ctrl-V blocking after the esc delay support from `master`.

Related: ClickHouse/replxx#44
Related: ClickHouse/replxx#46

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Comment thread src/terminal.cxx
Comment on lines +327 to +328
thread_local char8_t utf8String[5];
thread_local size_t utf8Count = 0;

@azat azat Oct 13, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not enough, multiple terminals may work over different fds, and this states should be per-fd, we need to move this state into the Terminal object

I've fixed it here - #47

alexey-milovidov added a commit that referenced this pull request Oct 13, 2026
`master` got a simpler version of the same fix (#44) and the esc delay support.
The conflicts are resolved in favor of this branch: the UTF-8 decoder state is
kept in `Terminal`, the escape sequence routines take the `Terminal`, and the
`SIGWINCH` disposition is restored. The esc delay parameter from `master` is
passed through, `Terminal::read_unicode_character` waits with `poll` as on
`master`, and `read_verbatim` still does not wait after the first character.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants