calcurse icon indicating copy to clipboard operation
calcurse copied to clipboard

notification.command freezes interactive mode UI

Open ilf opened this issue 3 years ago • 19 comments

calcurse 4.7.0 from Arch package, see config here: https://github.com/lfos/calcurse/issues/325#issuecomment-720140664

  1. Set up $notification.command
  2. Open calcurse interactive mode (in tmux).
  3. Wait until $notification.warning time (here: 5 minutes).
  4. Notification is displayed in bottom bar.
  5. The rest of the UI freezes. Keystrokes still work, but changes are not displayed. This leaves interactive mode useless (and dangerous), it needs to be restarted.

On a related note: When restarting calcurse between notification time and the event, the UI is still frozen. And $notification.command is executed twice (more).

ilf avatar Nov 02 '20 18:11 ilf

I haven't been able to reproduce this. Neither when starting calcurse before the notification time nor between notification time and event.

Have you tried running calcurse in an ordinary terminal (not tmux)? In #152 a problem with screen rendering was reported when using tmux.

The core dump in #325 was caused by an abort signal (SIGABRT). Did you issue that? How exactly do you invoke calcurse (complete command line).

You have the daemon enabled (daemon.enable=yes) which possibly explains the repeated notifications.

lhca avatar Nov 04 '20 07:11 lhca

Have you tried running calcurse in an ordinary terminal (not tmux)?

I have just tried calcurse directly in a terminal, without tmux. Same thing.

How exactly do you invoke calcurse (complete command line).

% which calcurse
/usr/bin/calcurse

% calcurse

You have the daemon enabled (daemon.enable=yes) which possibly explains the repeated notifications.

Is that a bug or a feature? I consider repeated notifications unnecessary.

ilf avatar Nov 04 '20 08:11 ilf

I think this might be a side effect of how we are running external commands. Notification commands should not take any input and should not print anything to stdout -- we are enforcing this for hooks but I don't think we have a similar enforcement for notification commands. To confirm, could you please append <&- >&- 2>&- to your notification command line?

lfos avatar Nov 04 '20 11:11 lfos

I think this might be a side effect of how we are running external commands.

I don't think so. Notification commands are not run by the main calcurse thread like hooks or external commands, but by the notify thread. In the case at hand there is no interaction with the terminal and if something were written to the screen it would just mess it up, not cause a deadlock.

lhca avatar Nov 04 '20 14:11 lhca

@ilf, did you use earlier versions of calcurse without this problem?

I assume that you killed the deadlocked process with a SIGABRT signal?

Calcurse is a multithreaded program and from your description it seems that calcurse is partly running and partly deadlocked. What does ps -eLf | grep calcurse show?

lhca avatar Nov 04 '20 14:11 lhca

@ilf, did you use earlier versions of calcurse without this problem?

I have had this bug in all versions I used: 4.5.1, 4.6.0 and now 4.7.0.

I assume that you killed the deadlocked process with a SIGABRT signal?

Since "keystrokes still work, but changes are not displayed", I am able to exit via q -> y and Ctrl+D.

What does ps -eLf | grep calcurse show?

Before exiting:

user       192402  153776  192402  1    4 16:09 pts/11   00:00:00 calcurse
user       192402  153776  192405  0    4 16:09 pts/11   00:00:00 calcurse
user       192402  153776  192407  0    4 16:09 pts/11   00:00:00 calcurse
user       192402  153776  192408  0    4 16:09 pts/11   00:00:00 calcurse

After:

user       192437       1  192437  0    1 16:09 ?        00:00:00 calcurse

ilf avatar Nov 04 '20 15:11 ilf

To confirm, could you please append <&- >&- 2>&- to your notification command line?

It seems that with this, the UI does not lock.

It's weird though, because calling my $notification.command (calcurse --next | mail -s "[calendar] upcoming event" user) does not output anything to stdout or stderr.

ilf avatar Nov 04 '20 15:11 ilf

It seems that with this, the UI does not lock.

But the mail sent from $notification.command does not have a body any more…

ilf avatar Nov 04 '20 16:11 ilf

I assume that you killed the deadlocked process with a SIGABRT signal?

Where did the core dump come from?

Since "keystrokes still work, but changes are not displayed", I am able to exit via q -> y and Ctrl+D.

Is the clock ticking in the notification bar?

What doesn't work? Please give a detailed example.

lhca avatar Nov 04 '20 18:11 lhca

Where did the core dump come from?

That's a different issue. (I think.)

ilf avatar Nov 04 '20 18:11 ilf

What doesn't work? Please give a detailed example.

I can try to make a screencast video.

But I think you should be able to reproduce it with notification.command=calcurse --next | mail -s "[calendar] upcoming event" user with mail being mailx from https://www.sdaoden.eu/code.html#s-nail, see https://wiki.archlinux.org/index.php/S-nail

Does that work?

ilf avatar Nov 04 '20 19:11 ilf

Thanks both for investigating.

The fact that the external command is executed from another thread doesn't mean that it cannot interact with the UI in ways other than overwriting the screen content. If certain escape sequences are printed by the command, ncurses will no longer function properly. A command that captures stdin can prevent keystrokes from reaching calcurse. Both effects can be demonstrated by using reset or read line as notification commands, respectively. Even though appending <&- >&- 2>&- breaks the pipe, the fact that it makes the issue disappear indicates that something along those lines is happening. A more careful investigation of how mail interacts with the terminal is probably required to confirm and investigate this more carefully. Can you try a different notification command that doesn't involve mail(1)?

lfos avatar Nov 04 '20 22:11 lfos

The description "the UI freezes" and the coredump lead me to believe that calcurse was deadlocked and had to be killed. This is apparently not the case. In stead, it seems to be a matter of screen update/rendering.

But I think you should be able to reproduce it with notification.command=calcurse --next | mail -s "[calendar] upcoming event" user with mail being mailx from https://www.sdaoden.eu/code.html#s-nail, see https://wiki.archlinux.org/index.php/S-nail

Are you saying that "mail" is really "mailx"?

In that case, if your system has /bin/mail, please try it:

notification.command=calcurse --next | /bin/mail -s "[calendar] upcoming event" user

lhca avatar Nov 05 '20 06:11 lhca

Are you saying that "mail" is really "mailx"?

All 4 (/usr)/bin/mail(x) are identical:

mail v14.9.19, 2020-04-26 (built for Linux)

I do not have a different mail.

Is the clock ticking in the notification bar?

Yes. The clock is ticking. During notification.warning time (5 minutes), the notification is blinking. After that, the next notification is displayed.

But when I try to navigate by pressing "Up" and "Down" keys, , the escape characters ^[OA and ^[OA are printed in the notification bar next to the blinking countdown, instead of actually navigating.

ilf avatar Nov 05 '20 08:11 ilf

This sounds pretty much the same as what you'd get when using reset as notification command. It is likely that mail interferes with the output, e.g. by printing escape sequences. Can you please try to append >&- 2>&- to the command line (without <&- as I suggested previously such that the pipe does not break)?

lfos avatar Nov 05 '20 11:11 lfos

Yes, adding >&- 2>&- fixes the issue.

(Sorry for the earlier contrary post, I hadn't restarted calcurse for the change to take effect.)

Why does calcurse care about anything notification.command returns? Seems like a potential security issue, no?

(I also don't understand how the value of notification.command is escaped, if at all, but that's not the point now.)

ilf avatar Nov 05 '20 15:11 ilf

@lfos:

The fact that the external command is executed from another thread doesn't mean that it cannot interact with the UI in ways other than overwriting the screen content.

True. I presumed that calcurse was deadlocked. This has turned out not to be the case. All threads are there, the main thread and the notify thread are both running.

Even though appending <&- >&- 2>&- breaks the pipe, the fact that it makes the issue disappear indicates that something along those lines is happening.

True again. It is reasonable to conclude that the mail command has interfered with the tty, something which calcurse cannot be blamed for.

This experience has made me think that maybe hooks should be run without closing stdin, stdout and stderr behind the user's back for the following reason. A hook script (or a notification command) should not tamper with those file descriptors, but if it does, it is in most cases much easier to detect, when they are not closed by calcurse.

In the present case, had the notification command been run with the file descriptors closed, it would have been very difficult to troubleshoot and would have required a recompilation to confirm that mail (probably) interfers with the terminal. On the other hand, if a hook for example writes to stdout, it is immediately visible on the screen and the user can rectify the hook.

lhca avatar Nov 05 '20 17:11 lhca

Why does calcurse care about anything notification.command returns? Seems like a potential security issue, no?

calcurse does not care about what notification.command "returns". The notification command is printing output to the terminal, partially overwriting screen content. We can suppress this by closing stdin/stdout/stderr before execution.

In the present case, had the notification command been run with the file descriptors closed, it would have been very difficult to troubleshoot and would have required a recompilation to confirm that mail (probably) interfers with the terminal. On the other hand, if a hook for example writes to stdout, it is immediately visible on the screen and the user can rectify the hook.

I am not sure I follow completely. In the present case, had the notification command been run with the file descriptors closed, the issue would not have appeared at all (as per ilfs last post).

The issue has also shown that appending <&- >&- 2>&- is not a good approach for closing those file descriptors since it breaks if a pipe is used in the command. We could add parentheses to create a subshell but there's probably another easier solution: do not modify the command line and pass pointers to two (otherwise unused) ints to shell_exec(), and maybe add a third one for stderr.

lfos avatar Nov 06 '20 12:11 lfos

@ilf There are a couple of commits in pu that should prevent external commands from interacting with the terminal (without the need for any workaround).

lfos avatar Apr 05 '21 14:04 lfos