"Drew Adams" <drew.adams@oracle.com> writes: > IIUC, `org-open-file' and its associated code, such as `org-file-apps', > `org-default-apps', and `org-apps-regexp-alist', have nothing > particularly to do with Org mode. They constitute general-purpose code > for opening files using associated programs. Code that uses them > should not need to load the Org code, and this code should be > maintained separately for general use. `org-open-file' is too Org-specific to move out of Org. But there should be a command to do what it essentially does outside of Org -- that is, use ~/.mailcap to determine what viewer to use. So I've now added this to Emacs 28 under the name `mailcap-view-file'. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
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 sorry if it is a false alarm. Feel free to close the bug again if something changed recently in `start-process-shell-command' or if you prefer to discuss the issue as another bug. It seems that implementation of `mailcap-view-file' is unreliable due to creation of unnecessary terminal session and it can cause obscure and difficult to reproduce failures similar to https://lists.gnu.org/archive/html/emacs-orgmode/2020-09/msg00195.html https://lists.gnu.org/archive/html/emacs-orgmode/2020-06/msg00332.html The thread is actually longer than it is shown in the archive interface. Another lengthy discussion: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=44824 In Org latest change was required for compatibility reason: https://code.orgmode.org/bzg/org-mode/commit/869b7a21b94ed112f6640c8f2711c2a68b661dea Let-bind (process-connection-type nil) is a minimal required change to avoid unnecessary terminal session. However it is not friendly to users in the case when troubleshooting is required. `make-process' with sentinel is hopefully better. The following could be ignored since it likely requires significant amount of work with unclear benefits. 1. `org-open-file' besides Org-specific stuff allows to specify precise target inside the file. It can be quite useful, e.g. okular --page 11 --find "some pattern" file.pdf PDF files have internal anchors as well. I have no consistent vision how to express additional "locators" in general API. 2. There are at least two sources of truth for MIME-handlers on linux desktop that are not necessary synchronized. Info from extracted from .desktop files may be configurable from desktop UI unlike mailcap. Distros may have some instruments to mitigate discrepancies. Debian adds entries from .desktop handlers to system-wide mailcap DB. Another approach is to add to maicap greedy xdg-open handler that tries to guess currently running desktop and pass arguments to appropriate command. Maybe mailcap should be secondary MIME database in Emacs, not the primary one. 3. Currently only file suffix is inspected to determine MIME type of a file. libmagic (or file command) usually provides more precise info, so it is possible to open an incorrectly named file. 4. Mailcap has more features that are not addressed in Emacs. They may be handy if Emacs is launched in terminal on remote server. It might allow e.g. to open PDF file using pdftotext handler. - A buffer for command output should be created for "copiousoutput" option. - A buffer should be created and terminal session should be enabled if an entry "needsterminal". - There are more substitutions than "%s". However I am unsure if it is possible to provide more info than application can obtain from the file. I think, it is intended for mail multipart messages and additional headers. On the other hand mailcap handlers might expect safe file names (minimal ASCII subset), users may have files with arbitrary names (national charset or some special characters). I hope, almost all handlers do not have such problem. In summary, during launch of external command terminal session must be suppressed. There is enough room for MIME-related improvements in Emacs in general and in Org mode in particular.
[-- Attachment #1: Type: text/plain, Size: 268 bytes --] 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. [-- Attachment #2: 0001-mailcap.el-Avoid-xdg-open-silent-failure.patch --] [-- Type: text/x-patch, Size: 2822 bytes --] From de55b623810736df04641a4d8f6027ccb04caa7f Mon Sep 17 00:00:00 2001 From: Max Nikulin <manikulin@gmail.com> 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
> From: Maxim Nikulin <manikulin@gmail.com>
> 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.
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.
> From: Maxim Nikulin <manikulin@gmail.com>
> Date: Fri, 2 Jul 2021 19:21:55 +0700
> Cc: Lars Ingebrigtsen <larsi@gnus.org>
>
> > 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?
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".
> From: Maxim Nikulin <manikulin@gmail.com>
> 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.
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.
> From: Maxim Nikulin <manikulin@gmail.com> > 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.
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.
> From: Maxim Nikulin <manikulin@gmail.com>
> 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?
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.
> From: Maxim Nikulin <manikulin@gmail.com> > 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?
Maxim Nikulin <manikulin@gmail.com> writes: > 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. Thanks; makes sense to me (and seems to fix these persistent issues with xgd-open etc), so I've applied it to Emacs 28. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no
On 30/07/2021 19:01, Lars Ingebrigtsen wrote: > Maxim Nikulin writes: > >> 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. > > Thanks; makes sense to me (and seems to fix these persistent issues with > xgd-open etc), so I've applied it to Emacs 28. Thanks for looking into this issue. Please, consider the following additional change: ----- 8< ----- diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el index f64897ac9b..aeeb9bd8d3 100644 --- a/lisp/net/mailcap.el +++ b/lisp/net/mailcap.el @@ -1186,7 +1186,6 @@ See \"~/.mailcap\", `mailcap-mime-data' and related files and variables." (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)) ---- >8 ---- I did not update the patch since Eli had some objections (unclear to me). I wrongly added :noquery expecting to get a process completely detached from Emacs, something like result of "synchronous" (process-file-shell-command command nil 0 nil) on linux (on stackoverflow I have seen a note that w32 requires special treatment). Notice that "pipe" asynchronous Emacs processes have a problem with CPU consumption if a process does something like #!/bin/sh exec 1>&- exec 2>&- sleep 30 Though I believe even silently killed on exit children and CPU-hungry Emacs in rare cases are better than decade old pseudoterminal+SIGHUP problem with xdg-open. I suppose, it is an Emacs bug, note Gnome or KDE issue. https://lists.gnu.org/archive/html/emacs-devel/2009-07/msg00279.html
Maxim Nikulin <manikulin@gmail.com> writes: > Thanks for looking into this issue. Please, consider the following > additional change: Thanks; applied to Emacs 28. -- (domestic pets only, the antidote for overdose, milk.) bloggy blog: http://lars.ingebrigtsen.no