Repository navigation
Add Io\Terminal native terminal primitives - #23941
prateekbhujel wants to merge 48 commits into
Conversation
| HANDLE handle = input; | ||
| DWORD mode; | ||
| WCHAR high_surrogate = 0; | ||
| DWORD raw_mode; | ||
| smart_str secret = {0}; | ||
| bool success = false; | ||
| bool failed = false; | ||
| bool mode_changed = false; |
There was a problem hiding this comment.
Reduce the scope of variables as much as possible.
TimWolla
left a comment
There was a problem hiding this comment.
I'm only remarking the first occurrence of any given point. Please carefully check if the same suggestion would apply elsewhere for everything.
| static void php_io_terminal_create_terminal_size(zval *return_value, zend_long cols, zend_long rows) | ||
| { | ||
| object_init_ex(return_value, php_io_terminal_terminal_size_ce); | ||
| zend_update_property_long(php_io_terminal_terminal_size_ce, Z_OBJ_P(return_value), "cols", sizeof("cols") - 1, cols); | ||
| zend_update_property_long(php_io_terminal_terminal_size_ce, Z_OBJ_P(return_value), "rows", sizeof("rows") - 1, rows); | ||
| } |
There was a problem hiding this comment.
Consider object_init_with_constructor() instead of a custom helper.
There was a problem hiding this comment.
Removed the internal initialization helper; Terminal::getSize() now instantiates TerminalSize via object_init_with_constructor().
| int fd = input; | ||
| struct termios mode; | ||
| struct termios raw_mode; | ||
| smart_str secret = {0}; | ||
| bool success = false; | ||
| bool mode_changed; | ||
| php_poll_ctx *poll_ctx; |
There was a problem hiding this comment.
Narrowing the scope is possible for a number of variables here (probably also in other functions).
There was a problem hiding this comment.
Narrowed variable declarations to their point of first use throughout io_terminal.c.
| #if SIZEOF_TIME_T < 8 | ||
| if (timeout_duration->duration.seconds > PHP_IO_TERMINAL_TIME_T_MAX) { | ||
| timeout.tv_sec = PHP_IO_TERMINAL_TIME_T_MAX; | ||
| } else { | ||
| timeout.tv_sec = (time_t) timeout_duration->duration.seconds; | ||
| } | ||
| #else | ||
| timeout.tv_sec = (time_t) timeout_duration->duration.seconds; |
There was a problem hiding this comment.
We don't have any other SIZEOF_TIME_T in the codebase, is this an issue that also affects the polling API? Should an exception being thrown for out-of-range Durations instead of silently clamping them?
There was a problem hiding this comment.
Replaced #if SIZEOF_TIME_T < 8 with a static inline clamping helper using sizeof(time_t) < 8.
Regarding clamping vs throwing: php_poll.c / io_poll.c takes the same approach when a Duration overflows INT_MAX milliseconds, capping rather than throwing because a timeout exceeding the platform limit is practically indefinite. The clamping helper keeps that behavior consistent between the two I/O extensions.
| #if !defined(PHP_WIN32) && defined(SIGWINCH) && defined(ZTS) | ||
| php_io_terminal_resize_mutex = tsrm_mutex_alloc(); | ||
| #endif | ||
| #if !defined(PHP_WIN32) && !defined(PHP_IO_TERMINAL_HAVE_PTSNAME_R) && defined(ZTS) | ||
| php_io_terminal_ptsname_mutex = tsrm_mutex_alloc(); | ||
| #endif |
There was a problem hiding this comment.
For readability it probably makes sense to poll the ZTS check to the beginning of the condition (or as a separate condition wrapping the internal conditions).
There was a problem hiding this comment.
Grouped the ZTS checks into outer #ifdef ZTS blocks in MINIT/MSHUTDOWN and simplified the mutex lock/unlock helpers into clean macros.
- Check EG(exception) after readonly property assignment in TerminalSize::__construct - Instantiate TerminalSize with object_init_with_constructor in Terminal::getSize - Match POSIX/Windows terminal identities by unique device/handle, avoiding raw descriptor reuse - Verify actual terminal state after tcsetattr and GetConsoleMode - Protect readSecret against NULL deref on disconnect, abort on control/escape sequences, and preserve fragmented UTF-8 across boundaries - Account for timeouts with zend_hrtime monotonic deadline across escape and multibyte decoding - Narrow variable declarations and simplify ZTS mutex macro guards
…cret timeouts - Fix cancellation inside readSecret() escape parsing: - Abort on Ctrl+C, Ctrl+D, Escape, Enter inside CSI and SS3 sequences via terminal restoration path. - Distinguish cancellation, EOF, internal 25ms sequence timeout (which ends sequence skipping and continues secret input), and overall secret timeout. - Discard valid CSI (e.g. arrow keys) and SS3 (e.g. F1) sequences without exiting or emitting garbage. - Enforce one monotonic read deadline in readKey(): - Cap sequence timeout and UTF-8 continuation reads by remaining overall budget. - Zero timeout executes non-blocking read without blocking wait even if sequenceTimeout is specified. - Incomplete escape sequences return raw byte strings; incomplete UTF-8 sequences retain pending bytes across calls and return null on timeout. - Adopt null for absence: - Update Terminal::getSize() return type to ?TerminalSize. - Update Terminal::readKey() return type to Key|string|null. - Keep Terminal::restoreMode() return type as bool. - Add optional overall timeout to Terminal::readSecret(?Time\Duration $timeout = null): - Returns null on timeout, '' on empty submission (immediate Enter). - Throws TerminalException on cancellation/failure. - Throws ValueError on negative duration. - Introduce mockable boundaries: - Add Io\Terminal\TerminalInterface and Io\Terminal\ModeTokenInterface. - Make native Terminal implement TerminalInterface and ModeToken implement ModeTokenInterface while remaining final. - Have native restoreMode(?ModeTokenInterface $mode = null) validate native ModeToken, rejecting foreign tokens with ValueError. - Add comprehensive regression and acceptance tests.
DanielEScherzer
left a comment
There was a problem hiding this comment.
some initial thoughts
| # define PATH_MAX 1024 | ||
| #endif | ||
|
|
||
| static bool php_io_terminal_get_slave_dev_from_pty_master(int fd, dev_t *slave_dev) |
There was a problem hiding this comment.
please use more inclusive names in new code, i.e. avoid "slave" and "master"
There was a problem hiding this comment.
Updated the new PTY identifiers to neutral terminology; the platform ptsname*() APIs remain unchanged.
| if (GetConsoleMode(shared->restore_stream, &actual_mode) && actual_mode != shared->saved_mode) { | ||
| return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
both blocks end with return true, maybe move that out of the preprocessor conditional?
There was a problem hiding this comment.
Done — moved the common return true outside the conditional.
|
|
||
| if (intern->valid && intern->shared != NULL) { | ||
| const char *err = NULL; | ||
| if (!php_io_terminal_release_token_lease(intern, &err)) { |
There was a problem hiding this comment.
this error isn't used for anything, so can just pass NULL
There was a problem hiding this comment.
Updated — passing NULL directly since the error output isn't used here.
| HANDLE handle = input; | ||
| DWORD mode = 0; | ||
| WCHAR high_surrogate = *pending_high_surrogate; | ||
| DWORD raw_mode; |
There was a problem hiding this comment.
some of these are not needed for a while, e.g. raw_mode can be declared as part of its initialization on 998
also applies in a number of other places, not going to point out each time
There was a problem hiding this comment.
Narrowed raw_mode, key, and repeats to their points of initialization.
| } | ||
| mode_changed = raw_mode != mode; | ||
|
|
||
| for (;;) { |
There was a problem hiding this comment.
probably clearer to use
| for (;;) { | |
| while (true) { |
There was a problem hiding this comment.
Updated to while (true).
|
|
||
| pending->bytes[pending->length++] = key; | ||
| if ((key & 0xc0) != 0x80) { | ||
| zend_string *invalid = zend_string_init((const char *) pending->bytes, pending->length, false); |
There was a problem hiding this comment.
I think this is just the same result as a break here
There was a problem hiding this comment.
Yep, this reaches the same common cleanup/return path. Simplified it to break.
| #define PHP_IO_TERMINAL_CSI_IS(literal) \ | ||
| (seq_len == sizeof(literal) - 1 && memcmp(seq, literal, sizeof(literal) - 1) == 0) | ||
|
|
||
| if (PHP_IO_TERMINAL_CSI_IS("\x1b[A")) return ZSTR_INIT_LITERAL("up", false); |
There was a problem hiding this comment.
a bunch of these have the same length in the comparison, and so this is going to have a whole bunch of unneeded comparisons of the seq_len vs the potential key. Maybe separate them out?
#define PHP_IO_TERMINAL_CSI_IS(literal) (memcmp(seq, literal, strlen(literal) == 0)
if (seq_len == strlen("\x1b[B")) {
if (PHP_IO_TERMINAL_CSI_IS("\x1b[B")) return ZSTR_INIT_LITERAL("down", false);
if (PHP_IO_TERMINAL_CSI_IS("\x1b[C")) return ZSTR_INIT_LITERAL("right", false);
if (PHP_IO_TERMINAL_CSI_IS("\x1b[D")) return ZSTR_INIT_LITERAL("left", false);
// ...
} else if (seq_len == strlen("\x1b[3~")) {
if (PHP_IO_TERMINAL_CSI_IS("\x1b[3~")) return ZSTR_INIT_LITERAL("delete", false);
if (PHP_IO_TERMINAL_CSI_IS("\x1b[5~")) return ZSTR_INIT_LITERAL("pageup", false);
if (PHP_IO_TERMINAL_CSI_IS("\x1b[6~")) return ZSTR_INIT_LITERAL("pagedown", false);
} else if (seq_len == strlen("\x1b[11~")) {
if (PHP_IO_TERMINAL_CSI_IS("\x1b[11~")) return ZSTR_INIT_LITERAL("f1", false);
if (PHP_IO_TERMINAL_CSI_IS("\x1b[12~")) return ZSTR_INIT_LITERAL("f2", false);
if (PHP_IO_TERMINAL_CSI_IS("\x1b[13~")) return ZSTR_INIT_LITERAL("f3", false);
// ...
}
but then the memory comparison against \x1b[ is repeated a whole bunch, and this can be further optimized
not urgent since this still needs an RFC, just noting
There was a problem hiding this comment.
Grouped the known sequences by length so we avoid repeating the length comparison for every candidate. The mappings and fallback behavior are unchanged.
| * @generate-c-enums | ||
| */ | ||
|
|
||
| namespace Io\Terminal { |
There was a problem hiding this comment.
you can just have a semicolon declaration and then don't need to indent everything
There was a problem hiding this comment.
Changed the stub to the semicolon namespace form and regenerated arginfo.
There was a problem hiding this comment.
I would recommend against this change: It adds unnecessary churn when later introducing sub-namespaces. For ext/date/time.stub.php I specifically started using the brace version right away.
There was a problem hiding this comment.
Ah, I hadn’t considered the future sub-namespace case when I applied Daniel’s suggestion. I was looking at it as a straightforward cleanup for the current single namespace. The ext/date/time.stub.php precedent makes the reason for keeping the braces clear. I’ve switched it back and regenerated the generated headers.
|
FWIW the timespec functions may be useful to extract into another utility file in the future. poll has Also, perhaps it might be useful to split the Unix/Windows backends into their own files, if it gets unwieldy. |
|
Yeah, both make sense. I kept them local for now since the terminal implementation is still fairly self-contained, but if the timespec helpers start being reused elsewhere, pulling them into a shared utility would be cleaner. Same for the platform code, if the Unix/Windows sides keep growing, splitting them out would probably make the implementation easier to maintain. Thanks for pointing it out. |
…m handling - Refactor public API to final Terminal and ModeToken classes under Nicolas's shared lease model - Implement explicit mode restoration via restoreMode(ModeToken $mode): void with transactional rollback - Implement bounded readiness and overall sequence deadlines for ordinary streams on POSIX and Windows - Unify byte-stream readSecret cancellation (Escape, Ctrl+C, Ctrl+D) and SS3/CSI controls across platforms - Harden CRLF lookahead across secret and line reading, preserving data CR before CRLF and at EOF - Isolate test seam hooks to debug/test builds (ZEND_DEBUG || PHP_TERMINAL_TEST_BUILD) - Add regression PHPT test suite covering fork isolation, failure injection, output flags, and streams
Drop the readLine() suppression and the EIO-to-EOF conversion from fe88929; they were broader than the CI failure required and could hide real read errors.
… raw mode ISIG checks
Mark enableRawMode() with NoDiscard so accidental disposal of the lease is visible. Document token destruction, cover the warning and explicit discard, and remove two setup calls that discarded their tokens.
Implements the Io\Terminal RFC for PHP 8.7.
The proposed API provides terminal size queries, raw-mode leases, key input, line input, and hidden input for POSIX terminals and Windows consoles. It also accepts ordinary streams for scripted input.
Raw-mode leases are coordinated per logical terminal. They can be released out of order or through another wrapper, and the final release restores the saved mode.
enableRawMode()is marked#[\NoDiscard]to warn when its returned token is accidentally discarded.The implementation includes PHPT coverage for mode ownership and restoration, key decoding, timeouts, resize handling, line and secret input, and transitions between reads.
The RFC remains under discussion.