From f557af9a9112955d3b65f6ad0d54c0791189f961 Mon Sep 17 00:00:00 2001 From: Robin Haberkorn Date: Tue, 9 May 2023 19:08:32 +0200 Subject: fixed CTRL+C interruptions on Windows; optimized CTRL+C polling on Gtk+ * teco_interrupt() turned out to be unsuitable to kill child processes (eg. when hangs). Instead, we have Win32-specific code now. * Since SIGINT can be ignored on UNIX, pressing CTRL+C was not guaranteed to kill the child process (eg. when hangs). At the same time, it makes sense to send SIGINT first, so programs can terminate gracefully. The behaviour has therefore been adapted: Interrupting with CTRL+C the first time will kill gracefully. The second time, a more agressive signal is sent to kill the child process. Unfortunately, this would be relatively tricky and complicated to do on Windows, so CTRL+C will always "hard-kill" the child process. * Moreover, teco_interrupt() killed the entire process on Windows when called the second time. This resulted in any interruption to terminate SciTECO unexpectedly when tried the second time on Gtk/Win32. * teco_sigint_occurred renamed to teco_interrupted: There may be several different sources for setting this flag. * Checking for CTRL+C on Gtk involves driving the main event loop repeatedly. This is a very expensive operation. We now do that only every 100ms. This is still sufficient since keyboard input comes from humans. This optimization saves 75% runtime on Windows and 90% on Linux. * The same optimization turned out to be contraproductive on PDCurses/WinGUI. --- configure.ac | 2 +- src/interface-curses/interface.c | 59 ++++++++++++++++++++--------------- src/interface-gtk/interface.c | 47 +++++++++++++++------------- src/main.c | 35 +++++---------------- src/sciteco.h | 5 +-- src/spawn.c | 67 ++++++++++++++++++++++++++++++++++------ 6 files changed, 126 insertions(+), 89 deletions(-) diff --git a/configure.ac b/configure.ac index 1dec488..b8e6e38 100644 --- a/configure.ac +++ b/configure.ac @@ -170,7 +170,7 @@ AC_CHECK_FUNCS([memset setlocale strchr strrchr fstat sscanf], , [ # glib defines G_OS_UNIX instead... case $host in *-*-linux* | *-*-*bsd* | *-*-darwin* | *-*-cygwin* | *-*-haiku*) - AC_CHECK_FUNCS([realpath fchown dup dup2 getpid open read mmap], , [ + AC_CHECK_FUNCS([realpath fchown dup dup2 getpid open read kill mmap], , [ AC_MSG_ERROR([Missing libc function]) ]) ;; diff --git a/src/interface-curses/interface.c b/src/interface-curses/interface.c index 25311c5..e056333 100644 --- a/src/interface-curses/interface.c +++ b/src/interface-curses/interface.c @@ -103,7 +103,7 @@ #define CURSES_TTY #endif -#if defined(PDCURSES_WINCON) || defined(NCURSES_WIN32) +#ifdef G_OS_WIN32 /** * This handler is the Windows-analogue of a signal @@ -119,7 +119,7 @@ teco_console_ctrl_handler(DWORD type) { switch (type) { case CTRL_C_EVENT: - teco_sigint_occurred = TRUE; + teco_interrupted = TRUE; return TRUE; } @@ -397,6 +397,16 @@ teco_interface_init(void) #ifndef CURSES_TTY teco_interface_init_clipboard(); #endif + + /* + * The default SIGINT signal handler seems to partially work + * as the console control handler. + * However, a second CTRL+C event (or raise(SIGINT)) would + * terminate the process. + */ +#ifdef G_OS_WIN32 + SetConsoleCtrlHandler(teco_console_ctrl_handler, TRUE); +#endif } GOptionGroup * @@ -675,14 +685,11 @@ teco_interface_init_interactive(GError **error) teco_interface_init_screen(); /* - * We must register this handler for - * asynchronous interruptions via CTRL+C - * reliably. The signal handler we already - * have won't do. - * Doing this here ensures that we have a higher - * precedence than the handler installed by PDCursesMod. + * We always have a CTRL handler on Windows, but doing it + * here again, ensures that we have a higher precedence + * than the one installed by PDCurses. */ -#if defined(PDCURSES_WINCON) || defined(NCURSES_WIN32) +#ifdef G_OS_WIN32 SetConsoleCtrlHandler(teco_console_ctrl_handler, TRUE); #endif @@ -1465,7 +1472,7 @@ teco_interface_popup_clear(void) gboolean teco_interface_is_interrupted(void) { - return teco_sigint_occurred != FALSE; + return teco_interrupted != FALSE; } #else /* !CURSES_TTY && !PDCURSES_WINCON && !NCURSES_WIN32 */ @@ -1473,27 +1480,29 @@ teco_interface_is_interrupted(void) /* * This function is called repeatedly, so we can poll the keyboard input queue, * filtering out CTRL+C. - * This is naturally very inefficient. * It's currently necessary as a fallback e.g. for PDCURSES_GUI or XCurses. + * + * NOTE: Theoretically, this can be optimized by doing wgetch() only every X + * microseconds like on Gtk+. + * But this turned out to slow things down, at least on PDCurses/WinGUI. */ gboolean teco_interface_is_interrupted(void) { - if (teco_interface.cmdline_window) { /* interactive mode */ - gint key; - - /* - * NOTE: getch() is configured to be nonblocking. - */ - while ((key = wgetch(teco_interface.cmdline_window)) != ERR) { - if (G_UNLIKELY(key == TECO_CTL_KEY('C'))) - return TRUE; - g_queue_push_tail(teco_interface.input_queue, - GINT_TO_POINTER(key)); - } + if (!teco_interface.cmdline_window) + /* batch mode */ + return teco_interrupted != FALSE; + + /* NOTE: getch() is configured to be nonblocking. */ + gint key; + while ((key = wgetch(teco_interface.cmdline_window)) != ERR) { + if (G_UNLIKELY(key == TECO_CTL_KEY('C'))) + return TRUE; + g_queue_push_tail(teco_interface.input_queue, + GINT_TO_POINTER(key)); } - return teco_sigint_occurred != FALSE; + return teco_interrupted != FALSE; } #endif @@ -1528,7 +1537,7 @@ teco_interface_blocking_getch(void) gint key = wgetch(teco_interface.cmdline_window); teco_memory_start_limiting(); /* allow asynchronous interruptions on */ - teco_sigint_occurred = FALSE; + teco_interrupted = FALSE; nodelay(teco_interface.cmdline_window, TRUE); #if defined(CURSES_TTY) || defined(PDCURSES_WINCON) || defined(NCURSES_WIN32) noraw(); /* FIXME: necessary because of NCURSES_WIN32 bug */ diff --git a/src/interface-gtk/interface.c b/src/interface-gtk/interface.c index d22009b..6301a71 100644 --- a/src/interface-gtk/interface.c +++ b/src/interface-gtk/interface.c @@ -70,6 +70,12 @@ static gboolean teco_interface_window_delete_cb(GtkWidget *widget, GdkEventAny * gpointer user_data); static gboolean teco_interface_sigterm_handler(gpointer user_data) G_GNUC_UNUSED; +/** + * Interval between polling for keypresses. + * In other words, this is the maximum latency to detect CTRL+C interruptions. + */ +#define TECO_POLL_INTERVAL 100000 /* microseconds */ + #define UNNAMED_FILE "(Unnamed)" #define USER_CSS_FILE ".teco_css" @@ -719,22 +725,27 @@ teco_interface_popup_clear(void) * system call overhead. * But the GDK lock that would be necessary for synchronization * has been deprecated. - * - * @todo It would be great to have platform-specific optimizations, - * so we can detect interruptions without having to drive the Glib - * event loop (e.g. using libX11 or Win32 APIs). - * There already is a keyboard hook for Win32 in interface-curses. - * On the downside, such solutions will probably freeze the window - * while SciTECO is busy. However we currently freeze the window - * anyway while being busy to avoid flickering. */ gboolean teco_interface_is_interrupted(void) { - if (gtk_main_level() > 0) - gtk_main_iteration_do(FALSE); + if (!gtk_main_level()) + /* batch mode */ + return teco_interrupted != FALSE; - return teco_sigint_occurred != FALSE; + /* + * By polling only every TECO_POLL_INTERVAL microseconds + * we save 75-90% of runtime. + */ + static guint64 last_poll_ts = 0; + guint64 now_ts = g_get_monotonic_time(); + + if (G_LIKELY(last_poll_ts+TECO_POLL_INTERVAL > now_ts)) + return teco_interrupted != FALSE; + last_poll_ts = now_ts; + + gtk_main_iteration_do(FALSE); + return teco_interrupted != FALSE; } static void @@ -1172,12 +1183,10 @@ teco_interface_key_pressed_cb(GtkWidget *widget, GdkEventKey *event, gpointer us gdk_keyval_to_upper(event->keyval) == GDK_KEY_C) /* * Handle asynchronous interruptions if CTRL+C is pressed. - * This will usually send SIGINT to the entire process - * group and set `teco_sigint_occurred`. * If the execution thread is currently blocking, * the key is delivered like an ordinary key press. */ - teco_interrupt(); + teco_interrupted = TRUE; else g_queue_push_tail(teco_interface.event_queue, gdk_event_copy((GdkEvent *)event)); @@ -1209,9 +1218,9 @@ teco_interface_key_pressed_cb(GtkWidget *widget, GdkEventKey *event, gpointer us */ gdk_window_freeze_updates(top_window); - teco_sigint_occurred = FALSE; + teco_interrupted = FALSE; teco_interface_handle_key_press(&event->key, &error); - teco_sigint_occurred = FALSE; + teco_interrupted = FALSE; gdk_window_thaw_updates(top_window); @@ -1255,12 +1264,6 @@ teco_interface_window_delete_cb(GtkWidget *widget, GdkEventAny *event, gpointer static gboolean teco_interface_sigterm_handler(gpointer user_data) { - /* - * Since this handler replaces the default signal handler, - * we also have to make sure it interrupts. - */ - teco_interrupt(); - /* * Similar to window deletion - emulate "close" key press. */ diff --git a/src/main.c b/src/main.c index c532294..467eb72 100644 --- a/src/main.c +++ b/src/main.c @@ -52,32 +52,13 @@ teco_int_t teco_ed = TECO_ED_AUTOEOL; -volatile sig_atomic_t teco_sigint_occurred = FALSE; - -#ifdef G_OS_UNIX - -void -teco_interrupt(void) -{ - /* - * This sends SIGINT to the entire process group, - * which makes sure that subprocesses are signalled, - * even when called from the wrong thread. - */ - if (kill(0, SIGINT)) - teco_sigint_occurred = TRUE; -} - -#else /* !G_OS_UNIX */ - -void -teco_interrupt(void) -{ - if (raise(SIGINT)) - teco_sigint_occurred = TRUE; -} - -#endif +/** + * Whether there was an asyncronous interruption (usually after pressing CTRL+C). + * However you should always use teco_interface_is_interrupted(), + * to check for interruptions because of its side effects. + * This variable is safe to set to TRUE from signal handlers and threads. + */ +volatile sig_atomic_t teco_interrupted = FALSE; /* * FIXME: Move this into file-utils.c? @@ -309,7 +290,7 @@ teco_initialize_environment(const gchar *program) static void teco_sigint_handler(int signal) { - teco_sigint_occurred = TRUE; + teco_interrupted = TRUE; } int diff --git a/src/sciteco.h b/src/sciteco.h index 4f3d88a..87bd973 100644 --- a/src/sciteco.h +++ b/src/sciteco.h @@ -85,10 +85,7 @@ enum { extern teco_int_t teco_ed; /* in main.c */ -extern volatile sig_atomic_t teco_sigint_occurred; - -/* in main.c */ -void teco_interrupt(void); +extern volatile sig_atomic_t teco_interrupted; /* * Allows automatic cleanup of FILE pointers. diff --git a/src/spawn.c b/src/spawn.c index 416c433..d79259e 100644 --- a/src/spawn.c +++ b/src/spawn.c @@ -19,8 +19,18 @@ #include "config.h" #endif +#include + #include +#include +#include + +#ifdef HAVE_WINDOWS_H +#define WIN32_LEAN_AND_MEAN +#include +#endif + #include "sciteco.h" #include "interface.h" #include "undo.h" @@ -60,8 +70,10 @@ static struct { GMainContext *mainctx; GMainLoop *mainloop; GSource *idle_src; + GPid pid; GSource *child_src; GSource *stdin_src, *stdout_src; + gboolean interrupted; teco_int_t from, to; teco_int_t start; @@ -220,10 +232,13 @@ teco_state_execute_done(teco_machine_main_t *ctx, const teco_string_t *str, GErr /* * NOTE: With G_SPAWN_LEAVE_DESCRIPTORS_OPEN and without G_SPAWN_SEARCH_PATH_FROM_ENVP, * Glib offers an "optimized codepath" on UNIX. - * G_SPAWN_SEARCH_PATH_FROM_ENVP does not appear to work on Windows, anyway. */ static const GSpawnFlags flags = G_SPAWN_DO_NOT_REAP_CHILD | +#ifdef G_OS_UNIX G_SPAWN_SEARCH_PATH | G_SPAWN_LEAVE_DESCRIPTORS_OPEN | +#else + G_SPAWN_SEARCH_PATH_FROM_ENVP | +#endif G_SPAWN_STDERR_TO_DEV_NULL; if (ctx->mode > TECO_MODE_NORMAL) @@ -257,17 +272,17 @@ teco_state_execute_done(teco_machine_main_t *ctx, const teco_string_t *str, GErr if (!envp) goto gerror; - GPid pid; gint stdin_fd, stdout_fd; - if (!g_spawn_async_with_pipes(NULL, argv, envp, flags, NULL, NULL, &pid, + if (!g_spawn_async_with_pipes(NULL, argv, envp, flags, NULL, NULL, &teco_spawn_ctx.pid, &stdin_fd, &stdout_fd, NULL, error)) goto gerror; - teco_spawn_ctx.child_src = g_child_watch_source_new(pid); + teco_spawn_ctx.child_src = g_child_watch_source_new(teco_spawn_ctx.pid); g_source_set_callback(teco_spawn_ctx.child_src, (GSourceFunc)teco_spawn_child_watch_cb, NULL, NULL); g_source_attach(teco_spawn_ctx.child_src, teco_spawn_ctx.mainctx); + teco_spawn_ctx.interrupted = FALSE; #ifdef G_OS_WIN32 stdin_chan = g_io_channel_win32_new_fd(stdin_fd); @@ -343,11 +358,10 @@ teco_state_execute_done(teco_machine_main_t *ctx, const teco_string_t *str, GErr g_source_unref(teco_spawn_ctx.stdout_src); g_source_unref(teco_spawn_ctx.child_src); - g_spawn_close_pid(pid); + g_spawn_close_pid(teco_spawn_ctx.pid); /* - * NOTE: This includes interruptions due to teco_interrupt() - * following CTRL+C. + * NOTE: This includes interruptions following CTRL+C. * But they are reported as G_SPAWN_ERROR_FAILED and hard to filter out. */ if (teco_spawn_ctx.error) { @@ -487,6 +501,9 @@ gboolean teco_state_execute_process_edit_cmd(teco_machine_main_t *ctx, teco_mach * the completes, which may result in editor hangs. * You may however interrupt the spawned process by sending * the \fBSIGINT\fP signal to \*(ST, e.g. by pressing CTRL+C. + * The first time, this will try to kill the spawned process + * gracefully. + * The second time you press CTRL+C, it will hard kill the process. * * In interactive mode, \*(ST performs TAB-completion * of filenames in the string parameter but @@ -687,9 +704,39 @@ error: static gboolean teco_spawn_idle_cb(gpointer user_data) { - if (G_UNLIKELY(teco_interface_is_interrupted())) - /* Just in case this didn't yet happen. Should kill the child process. */ - teco_interrupt(); + if (G_LIKELY(!teco_interface_is_interrupted())) + return G_SOURCE_CONTINUE; + teco_interrupted = FALSE; + +#ifdef G_OS_WIN32 + /* + * FIXME: We could actually try to gracefully terminate the process first + * using GenerateConsoleCtrlEvent(CTRL_C_EVENT). + * However, it's hard to find the correct process group id. + * We can pass 0, but this will fire our own control handler again, + * resulting in a hard kill via TerminateProcess() anyway. + * Masking the control handler is also not viable because it's a race + * condition. All the workarounds would be very hacky. + * + * FIXME: TerminateProcess() does not work on processes that ignore their + * control handler after calling SetConsoleCtrlHandler(NULL, TRUE), + * even though it correctly sets its return code. + * SciTECO will currently simply hang on these processes - but you can + * kill the child process via the task manager. + */ + TerminateProcess(teco_spawn_ctx.pid, 1); +#elif defined(G_OS_UNIX) + /* + * The first CTRL+C will try to gracefully terminate the process. + */ + if (teco_spawn_ctx.interrupted || kill(teco_spawn_ctx.pid, SIGINT); + kill(teco_spawn_ctx.pid, SIGKILL); +#else + /* This may signal unrelated processes as well. */ + raise(SIGINT); +#endif + teco_spawn_ctx.interrupted = TRUE; + return G_SOURCE_CONTINUE; } -- cgit v1.2.3