prim_tty: Use `os:signal/2` to handle `SIGCONT` and `SIGWINCH` #8939

dumbbell · 2024-10-15T08:16:10Z

Why

Support for SIGCONT and SIGWINCH was added in previous commit to allow any Erlang application to react to them. Unfortunately, setting handlers for these two signals broke prim_tty which installed its own handlers in prim_tty_nif.c.

How

This patch changes prim_tty to use os:signal/2 along with its own gen_event callbacu module to handle the two signals from the Erlang code, like any other Erlang application.

The C code was removed because it is now unused.

github-actions · 2024-10-15T08:17:08Z

CT Test Results

4 files 200 suites 1h 50m 32s ⏱️
3 032 tests 2 742 ✅ 289 💤 1 ❌
3 963 runs 3 620 ✅ 342 💤 1 ❌

For more details on these failures, see this check.

Results for commit 7e7e588.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

dumbbell · 2024-10-15T08:21:50Z

In this patch, I could not remove the argument to tty_select/3 to make it tty_seject/2 (in both prim_tty.erl and prim_tty_nif.c). Otherwise, I get the following compilation failure:

gmake[3]: Entering directory '/home/dumbbell/Projects/erlang/GIT/lib/kernel/src'
 ERLC	../ebin/os.beam
 ERLC	../ebin/prim_tty.beam
 ERLC	../ebin/prim_tty_sighandler.beam
 VSN	../ebin/kernel.app
=WARNING REPORT==== 15-Oct-2024::10:20:33.211991 ===
The on_load function for module prim_tty returned:
{{case_clause,{error,{bad_lib,"Function not found prim_tty:tty_select/2"}}},
 [{prim_tty,on_load,1,[]},{code_server,'-handle_on_load/5-fun-0-',1,[]}]}

=ERROR REPORT==== 15-Oct-2024::10:20:33.211058 ===
Error in process <0.57.0> with exit value:
{{case_clause,{error,{bad_lib,"Function not found prim_tty:tty_select/2"}}},
 [{prim_tty,on_load,1,[]},{code_server,'-handle_on_load/5-fun-0-',1,[]}]}

=ERROR REPORT==== 15-Oct-2024::10:20:33.214625 ===
** State machine user_drv terminating
** When server state  = {undefined,undefined}
** Reason for termination = error:{'module could not be loaded',prim_tty}
** Callback modules = [user_drv]
** Callback mode = state_functions
** Stacktrace =
**  [{user_drv,init,1,[]},
     {gen_statem,init_it,6,[]},
     {proc_lib,init_p_do_apply,3,[]}]

I submitted the patch with tty_select/3 unused to share the patch and start the discussion.

dumbbell · 2024-10-15T08:25:11Z

Another point of discussion: the added prim_tty_sighandler module sends exactly the same message {ReaderRef, {signal, Signal}} to the Parent PID. I don’t know if that message format is part of a public API or not.

jhogberg · 2024-10-15T08:25:33Z

erts/emulator/nifs/common/prim_tty_nif.c


 static ErlNifFunc nif_funcs[] = {
    {"isatty", 1, isatty_nif},
    {"tty_create", 0, tty_create_nif},
    {"tty_init", 3, tty_init_nif},
    {"tty_set", 1, tty_set_nif},
-    {"tty_read_signal", 2, tty_read_signal_nif},
    {"setlocale", 1, setlocale_nif},
    {"tty_select", 3, tty_select_nif},


I submitted the patch with tty_select/3 unused to share the patch and start the discussion.

Change this to {"tty_select", 2, tty_select_nif}, and in that function, change argv[2] to argv[1] to account for the removed argument in the middle. :-)

I’m not sure I understood. I don’t see the difference in the {"tty_select", 3, tty_select_nif}, you typed compared to the code (I didn’t modify that line).

Do you suggest that the C code still exposes tty_select/3 but only uses the first two args, then the Erlang code calls it as tty_select/2?

Sorry, I noticed I hadn't updated it to 2 a few seconds after posting. Guess you missed my edit :-(

{"tty_select", 3, tty_select_nif}, means that tty_select/3 has to be present in the module for NIF loading to work (Edit: you can read it as "patch the function tty_select/3 with tty_select_nif").

If you want tty_select/2, change that line to {"tty_select", 2, tty_select_nif},.

That’s what I did yesterday but it fails to compile with the error pasted above.

I see that prim_tty.beam is present in the bootstrap. I looked at HOWTO/BOOTSTRAP.md but it says that it shouldn’t be modified during development.

Ah yes. To make this work while testing, run ./otp_build update_bootstrap --no-commit.

Ok. I don’t have any planned changes left, but I don’t know what to do with CI :-) Do you want me to mark the pull request as ready for review?

With ./otp_build update_primary --no-commit and an improvement to take into account that erl_signal_server may not be running, I can compile locally with tty_select/2. The interactive_shell_SUITE is still happy.

That's odd, unless I'm missing something erl_signal_server should've been started before user (and thereby user_sup -> user_drv -> prim_tty), and I'm not sure that ignoring its absence is the right thing to do. @garazdawi ?

Ok. I don’t have any planned changes left, but I don’t know what to do with CI :-) Do you want me to mark the pull request as ready for review?

Sure :-)

That's odd, unless I'm missing something erl_signal_server should've been started before user

That’s how I interpret the compile-time error: gen_event:delete_handler/3 throws a noproc error. Perhaps this is a specific situation during the compilation if prim_tty from bootstrap is used?

Because with the interactive_shell_SUITE or manual testing with an Erlang shell, I didn’t hit this error.

The signal server is not started when -mode minimal is passed, so that is why it fails with erlc as it uses that flag. I pushed a commit that moves when it is started so that it is always present.

dumbbell · 2024-10-15T08:30:17Z

Now I see that it fails to build on CI, even though interactive_shell_SUITE passes successfully locally. There must be something I didn’t get right with the build system :-)

garazdawi · 2024-10-15T20:14:44Z

I pushed some commits that fix some issues on windows and included an updated bootstrap so that github ci should work better now. Now time to sleep.

garazdawi · 2024-10-16T10:44:38Z

lib/kernel/src/prim_tty_sighandler.erl

+    ShortSignal = case Signal of
+                      sigcont  -> cont;
+                      sigwinch -> winch
+                  end,


There is no need to keep the old protocol, so you can send sigcont/sigwinch directly.

I updated the patch to address your feedback.

Note that I rebased the branch as I usually do and force-pushed. I’m sorry, I forgot that you had a local working copy, you already pushed commits and you may have uncommitted changes…

Great! No worries about the force push.

[Why] Support for SIGCONT and SIGWINCH was added in previous commit to allow any Erlang application to react to them. Unfortunately, setting handlers for these two signals broke `prim_tty` which installed its own handlers in `prim_tty_nif.c`. [How] This patch changes `prim_tty` to use `os:signal/2` along with its own gen_event callbacu module to handle the two signals from the Erlang code, like any other Erlang application. The C code was removed because it is now unused.

dumbbell marked this pull request as draft October 15, 2024 08:25

jhogberg reviewed Oct 15, 2024

View reviewed changes

dumbbell mentioned this pull request Oct 15, 2024

Add SIGWINCH and SIGINFO support #8887

Merged

dumbbell force-pushed the use-os-signal-2-in-prim_tty branch from 039d147 to 4c5a367 Compare October 15, 2024 11:56

dumbbell marked this pull request as ready for review October 15, 2024 12:34

garazdawi self-assigned this Oct 15, 2024

garazdawi reviewed Oct 16, 2024

View reviewed changes

dumbbell and others added 4 commits October 16, 2024 17:50

kernel: Guarantee that signal server will always run

dc90f45

kernel: Fix terminal window resize events on windows

f32ba53

Update primary bootstrap

7e7e588

dumbbell force-pushed the use-os-signal-2-in-prim_tty branch from 7ae182c to 7e7e588 Compare October 16, 2024 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prim_tty: Use `os:signal/2` to handle `SIGCONT` and `SIGWINCH` #8939

prim_tty: Use `os:signal/2` to handle `SIGCONT` and `SIGWINCH` #8939

dumbbell commented Oct 15, 2024

github-actions bot commented Oct 15, 2024 •

edited

Loading

dumbbell commented Oct 15, 2024

dumbbell commented Oct 15, 2024

jhogberg Oct 15, 2024 •

edited

Loading

dumbbell Oct 15, 2024

jhogberg Oct 15, 2024 •

edited

Loading

dumbbell Oct 15, 2024 •

edited

Loading

jhogberg Oct 15, 2024

dumbbell Oct 15, 2024

jhogberg Oct 15, 2024 •

edited

Loading

jhogberg Oct 15, 2024

dumbbell Oct 15, 2024

garazdawi Oct 15, 2024

dumbbell commented Oct 15, 2024

garazdawi commented Oct 15, 2024

garazdawi Oct 16, 2024

dumbbell Oct 16, 2024

garazdawi Oct 17, 2024

prim_tty: Use os:signal/2 to handle SIGCONT and SIGWINCH #8939

Are you sure you want to change the base?

prim_tty: Use os:signal/2 to handle SIGCONT and SIGWINCH #8939

Conversation

dumbbell commented Oct 15, 2024

Why

How

github-actions bot commented Oct 15, 2024 • edited Loading

CT Test Results

Artifacts

dumbbell commented Oct 15, 2024

dumbbell commented Oct 15, 2024

jhogberg Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhogberg Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

dumbbell Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhogberg Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dumbbell commented Oct 15, 2024

garazdawi commented Oct 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prim_tty: Use `os:signal/2` to handle `SIGCONT` and `SIGWINCH` #8939

prim_tty: Use `os:signal/2` to handle `SIGCONT` and `SIGWINCH` #8939

github-actions bot commented Oct 15, 2024 •

edited

Loading

jhogberg Oct 15, 2024 •

edited

Loading

jhogberg Oct 15, 2024 •

edited

Loading

dumbbell Oct 15, 2024 •

edited

Loading

jhogberg Oct 15, 2024 •

edited

Loading