bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
> From: Maxim Nikulin > Date: Mon, 5 Jul 2021 20:12:34 +0700 > > It is about proper way to a launch viewer in > `mailcap-view-file'. Original `start-process-shell-command' with 'pty > connection type prematurely kills children of kde-open5 or gio open. > With 'pipe connection type it or `make-process' might make emacs CPU > hungry if a child decides to close stdout and stderr: > > >> #!/bin/sh > >> exec 1>&- > >> exec 2>&- > >> sleep 30 Is the above something a file viewer is likely to do? And if it does, how would you suggest to countermand that? > and finally `process-file-shell-command' does not allow to report > failure. The original code uses start-process-shell-command, and I don't think I understand why you wanted to call process-file-shell-command instead. Can you explain?
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
By mistake I sent the message below as private one at first. However it actually does not add anything new to my previous comments to the bug. On 04/07/2021 20:49, Eli Zaretskii wrote: From: Maxim Nikulin Date: Sun, 4 Jul 2021 20:37:24 +0700 Sorry, I'm not sure I understand what this is all about. Are you still talking about the patch you proposed? Yes, I am. It is about proper way to a launch viewer in `mailcap-view-file'. Original `start-process-shell-command' with 'pty connection type prematurely kills children of kde-open5 or gio open. With 'pipe connection type it or `make-process' might make emacs CPU hungry if a child decides to close stdout and stderr: #!/bin/sh exec 1>&- exec 2>&- sleep 30 and finally `process-file-shell-command' does not allow to report failure. Moreover you suspect another secret compatibility problem with 'pipe.
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
> From: Maxim Nikulin > Date: Sun, 4 Jul 2021 20:37:24 +0700 > > I admit that I wrongly added ":noquery t", for some reason I believed > that it allows to choose whether processes are allowed to exist longer > than emacs or it is preferred to kill them with emacs. Actually > asynchronous processes are killed always and the option manages the > query only. This option should be dropped to restore compatibility with > previous variant. > > I have not found a way to detach asynchronous process from emacs. > Surprisingly it is possible for synchronous processes but it is > impossible to detect failure (thus to allow a user to figure out what > has happened) > > (process-file-shell-command command nil 0 nil) > > So process API in emacs is a kind of a short blanket. > > Accidentally I have created an example of program that is incompatible > with 'pipe asynchronous processes in emacs > > #!/bin/sh > exec 1>&- > exec 2>&- > sleep 30 > > (let ((command "cpu-stress-test") >(process-connection-type nil)) >(start-process-shell-command command nil command)) > > And emacs (at least 26.3) consumes 100% CPU for the specified amount of > time. I do not see any reason to do so since the program does not do > anything ugly. I have not found a way to explicitly force emacs to close > pipes. That is why I consider it as an outstanding bug. Emacs must > properly handle closed pipes. > > So `process-file-shell-command' ... 0 is better than current > `start-process-shell-command' but it does not allow to add error handling. > > So besides that I still have no guess what problem you suspect, now I > know that emacs may become mad in response to purely innocent action of > a child process. Sorry, I'm not sure I understand what this is all about. Are you still talking about the patch you proposed?
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
On 03/07/2021 18:56, Eli Zaretskii wrote: From: Maxim Nikulin Date: Sat, 3 Jul 2021 18:29:30 +0700 I am giving up with this issue. That's too bad. I see no reason to give up, and I urge you to reconsider, please. Sorry, but the space of your assumptions and maybe confusions has too high number of dimension to realize what you actually mean and what you consider as a problem that should be fixed. So any further steps are impossible. Your patch unconditionally assumes that every handler will immediately exit, and that it doesn't care about the connection type with the parent Emacs process, but that is not guaranteed to be true. There is no intention of such assumption and it *works* even for handlers that does not exit immediately. I admit that I wrongly added ":noquery t", for some reason I believed that it allows to choose whether processes are allowed to exist longer than emacs or it is preferred to kill them with emacs. Actually asynchronous processes are killed always and the option manages the query only. This option should be dropped to restore compatibility with previous variant. I have not found a way to detach asynchronous process from emacs. Surprisingly it is possible for synchronous processes but it is impossible to detect failure (thus to allow a user to figure out what has happened) (process-file-shell-command command nil 0 nil) So process API in emacs is a kind of a short blanket. Accidentally I have created an example of program that is incompatible with 'pipe asynchronous processes in emacs #!/bin/sh exec 1>&- exec 2>&- sleep 30 (let ((command "cpu-stress-test") (process-connection-type nil)) (start-process-shell-command command nil command)) And emacs (at least 26.3) consumes 100% CPU for the specified amount of time. I do not see any reason to do so since the program does not do anything ugly. I have not found a way to explicitly force emacs to close pipes. That is why I consider it as an outstanding bug. Emacs must properly handle closed pipes. So `process-file-shell-command' ... 0 is better than current `start-process-shell-command' but it does not allow to add error handling. So besides that I still have no guess what problem you suspect, now I know that emacs may become mad in response to purely innocent action of a child process.
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
> From: Maxim Nikulin > Date: Sat, 3 Jul 2021 18:29:30 +0700 > > I am giving up with this issue. That's too bad. I see no reason to give up, and I urge you to reconsider, please. > >> Because of I can not imagine such case for mailcap handler in Emacs yet > >> and, accordingly to you, "this could be an incompatible behavior change". > > > > You don't need to imagine it, you just need to trust me that I know > > what I'm talking about: it would be an incompatible change. > > Is it a kind of Church of Emacs that I have to just believe in you? It isn't a church, but some kind of trust cannot harm. > Previous time you were trying to convince me that unconditional 'pipe is > perfectly safe when I was unsure concerning behavior on Windows. It is indeed safe for Windows, because Emacs on Windows always uses pipes (as PTYs are not available there). My concern here is for systems other than Windows and other than those where you saw the issue. Your patch unconditionally assumes that every handler will immediately exit, and that it doesn't care about the connection type with the parent Emacs process, but that is not guaranteed to be true. What I'm asking is to let some kind of "fire escape" for users who could be adversely affected by this assumption. Ideally, some automatic detection of the handlers that need pipes would be the best. If that is not feasible, at least an option to control process-connection-type would be enough. > You prefer to keep reasons of your objections unveiled. I see no issue > with the patch. It can be by a few lines shorter but the price is worse > user experience. I have no idea how to move further. I explained the issue I have with unconditionally changing the interface. I have explained it above again. I hope it is clear enough. > Finally, the patch touches month-old unreleased code, so I see no point > to discuss that it is "incompatible". Hmm... that's true. So I guess an option to use PTYs should be good enough here. > P.S. It was my fault to use `make-process' in Org since the function is > not available in Emacs-24. I'm sorry for that incompatibility. Great, thanks. So I think it should be easy to adjust your patch to have a variable that controls process-connection-type, and then it could be installed.
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
I am giving up with this issue. My summary. New function `mailcap-view-file' has a problem similar to Bug#44824 (kde-open5 and "gio open" called directly or through xdg-open are unreliable in Emacs, I consider it as fixed in Org mode) that was reported by several users and refused by developers as not reproducible at first. I tried my best to draw attention to this problem and then to suggest a change that fixes the obscure problem and improves error handling. I am unaware of any real negative consequences of my patch. It is up to Emacs developers to leave the new bug as is or to fix it in a way they like On 03/07/2021 00:28, Eli Zaretskii wrote: From: Maxim Nikulin Date: Fri, 2 Jul 2021 23:24:23 +0700 On 02/07/2021 19:37, Eli Zaretskii wrote: From: Maxim Nikulin Date: Fri, 2 Jul 2021 19:21:55 +0700 And with other handlers, this could be an incompatible behavior change if the handler behaves differently when its standard handles are connected to a pipe rather than a terminal device. Example of such handler, please. Why do you need one? Because of I can not imagine such case for mailcap handler in Emacs yet and, accordingly to you, "this could be an incompatible behavior change". You don't need to imagine it, you just need to trust me that I know what I'm talking about: it would be an incompatible change. Is it a kind of Church of Emacs that I have to just believe in you? Previous time you were trying to convince me that unconditional 'pipe is perfectly safe when I was unsure concerning behavior on Windows. You prefer to keep reasons of your objections unveiled. I see no issue with the patch. It can be by a few lines shorter but the price is worse user experience. I have no idea how to move further. Finally, the patch touches month-old unreleased code, so I see no point to discuss that it is "incompatible". P.S. It was my fault to use `make-process' in Org since the function is not available in Emacs-24. I'm sorry for that incompatibility.
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
> From: Maxim Nikulin > Date: Fri, 2 Jul 2021 23:24:23 +0700 > > On 02/07/2021 19:37, Eli Zaretskii wrote: > >> From: Maxim Nikulin > >> Date: Fri, 2 Jul 2021 19:21:55 +0700 > >> Cc: Lars Ingebrigtsen > >> > >>> And with other handlers, this could be an > >>> incompatible behavior change if the handler behaves differently when > >>> its standard handles are connected to a pipe rather than a terminal > >>> device. > >> > >> Example of such handler, please. > > > > Why do you need one? > > Because of I can not imagine such case for mailcap handler in Emacs yet > and, accordingly to you, "this could be an incompatible behavior change". You don't need to imagine it, you just need to trust me that I know what I'm talking about: it would be an incompatible change. Is it possible to detect that the handler is one of those that are affected by this issue? If so, let's do that; it cannot be worse than what you suggested, or worse than the current situation.
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
On 02/07/2021 19:37, Eli Zaretskii wrote: From: Maxim Nikulin Date: Fri, 2 Jul 2021 19:21:55 +0700 Cc: Lars Ingebrigtsen And with other handlers, this could be an incompatible behavior change if the handler behaves differently when its standard handles are connected to a pipe rather than a terminal device. Example of such handler, please. Why do you need one? Because of I can not imagine such case for mailcap handler in Emacs yet and, accordingly to you, "this could be an incompatible behavior change".
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
> From: Maxim Nikulin > Date: Fri, 2 Jul 2021 19:21:55 +0700 > Cc: Lars Ingebrigtsen > > > And with other handlers, this could be an > > incompatible behavior change if the handler behaves differently when > > its standard handles are connected to a pipe rather than a terminal > > device. > > Example of such handler, please. Why do you need one?
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
On 02/07/2021 01:38, Eli Zaretskii wrote: From: Maxim Nikulin Date: Fri, 2 Jul 2021 00:01:59 +0700 --- a/lisp/net/mailcap.el +++ b/lisp/net/mailcap.el -(start-process-shell-command command nil command))) ... +(make-process + :name "mailcap-view-file" :connection-type 'pipe :noquery t + :buffer nil ; "*Messages*" may be suitable for debugging + :sentinel (lambda (proc event) + (when (and (memq (process-status proc) '(exit signal)) +(/= (process-exit-status proc) 0)) + (message +"Command %s: %s." +(mapconcat #'identity (process-command proc) " ") +(substring event 0 -1 + :command (list shell-file-name shell-command-switch command First, you replace start-process-shell-command with make-process, and I'm not sure I understand why. If all you want is to use pipes, why not simply bind process-connection-type around the call to start-process-shell-command? +;; An alternative is `process-connection-type' let-bound to nil for +;; `start-process-shell-command' call (with no chance to report failure). --- If main process of the handler fails then show a message with exit reason. ---^^ And with other handlers, this could be an incompatible behavior change if the handler behaves differently when its standard handles are connected to a pipe rather than a terminal device. Example of such handler, please.
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
> From: Maxim Nikulin > Date: Fri, 2 Jul 2021 00:01:59 +0700 > > --- a/lisp/net/mailcap.el > +++ b/lisp/net/mailcap.el > @@ -1177,7 +1177,23 @@ See \"~/.mailcap\", `mailcap-mime-data' and related > files and variables." > (shell-quote-argument (convert-standard-filename file)) > command > nil t)) > -(start-process-shell-command command nil command))) > +;; Handlers such as "gio open" and kde-open5 start viewer in background > +;; and exit immediately. Avoid `start-process' since it assumes > +;; :connection-type 'pty and kills children processes with SIGHUP > +;; when temporary terminal session is finished (Bug#44824). > +;; An alternative is `process-connection-type' let-bound to nil for > +;; `start-process-shell-command' call (with no chance to report failure). > +(make-process > + :name "mailcap-view-file" :connection-type 'pipe :noquery t > + :buffer nil ; "*Messages*" may be suitable for debugging > + :sentinel (lambda (proc event) > + (when (and (memq (process-status proc) '(exit signal)) > +(/= (process-exit-status proc) 0)) > + (message > +"Command %s: %s." > +(mapconcat #'identity (process-command proc) " ") > +(substring event 0 -1 > + :command (list shell-file-name shell-command-switch command I have two issues with this change: First, you replace start-process-shell-command with make-process, and I'm not sure I understand why. If all you want is to use pipes, why not simply bind process-connection-type around the call to start-process-shell-command? Does it not work for some reason? And second, I'm not sure we should make this change unconditionally. It isn't guaranteed that the handler will be one of those which have the problem, is it? And with other handlers, this could be an incompatible behavior change if the handler behaves differently when its standard handles are connected to a pipe rather than a terminal device. So I'd rather make this a conditional change, ideally only when one of the affected handlers is used (assuming we can detect that somehow). Thanks.
bug#12972: [PATCH] Avoid regression in mailcap-view-file similar to Bug#44824
On 01/06/2021 13:56, Lars Ingebrigtsen wrote: So I've now added this to Emacs 28 under the name `mailcap-view-file'. I am attaching a patch similar to proposed to Org mode that should help to avoid obscure failures of viewers due to unnecessary terminal sessions. >From de55b623810736df04641a4d8f6027ccb04caa7f Mon Sep 17 00:00:00 2001 From: Max Nikulin Date: Thu, 1 Jul 2021 23:41:16 +0700 Subject: [PATCH] mailcap.el: Avoid xdg-open silent failure * lisp/net/mailcap.el (mailcap-view-file): Use 'pipe :connection-type instead of 'pty to prevent killing of background process on handler exit. Avoid regression similar to Bug#44824. Problem happens only in some desktop environments where mailcap handler launches actual viewer (as defined in .desktop files and obtained from mimeapps.list) in background. E.g. xdg-open invokes "gio open" or kde-open5 for Gnome or KDE accordingly and these handlers launch e.g. eog or okular in background. As soon as main process exits, temporary terminal session created by `start-process-shell-command' is terminated. As a result background processes receive SIGHUP. Previously command were executed with no buffer as well, so the change does not affect "needsterminal" and "copiousoutput" mailcap features, they are not supported as earlier. If main process of the handler fails then show a message with exit reason. Output (including error messages) is ignored as before. Gtk applications tend to report significant amount of failed asserts hardly informative for majority of users. --- lisp/net/mailcap.el | 18 +- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el index 54f7f416ab..a53e385662 100644 --- a/lisp/net/mailcap.el +++ b/lisp/net/mailcap.el @@ -1177,7 +1177,23 @@ See \"~/.mailcap\", `mailcap-mime-data' and related files and variables." (shell-quote-argument (convert-standard-filename file)) command nil t)) -(start-process-shell-command command nil command))) +;; Handlers such as "gio open" and kde-open5 start viewer in background +;; and exit immediately. Avoid `start-process' since it assumes +;; :connection-type 'pty and kills children processes with SIGHUP +;; when temporary terminal session is finished (Bug#44824). +;; An alternative is `process-connection-type' let-bound to nil for +;; `start-process-shell-command' call (with no chance to report failure). +(make-process + :name "mailcap-view-file" :connection-type 'pipe :noquery t + :buffer nil ; "*Messages*" may be suitable for debugging + :sentinel (lambda (proc event) + (when (and (memq (process-status proc) '(exit signal)) +(/= (process-exit-status proc) 0)) + (message +"Command %s: %s." +(mapconcat #'identity (process-command proc) " ") +(substring event 0 -1 + :command (list shell-file-name shell-command-switch command (provide 'mailcap) -- 2.25.1
