bug#33050: 27.0.50; [macOS] Problem with process input with process-connection-type nil

[Thomas Fitzsimmons <fitzsim@fitzsim.org>]

[-- Attachment #1: Type: text/plain, Size: 2752 bytes --]

Filipp Gunbin <fgunbin@fastmail.fm> writes:

> Thomas,
>
> On 26/10/2018 11:41 -0400, Thomas Fitzsimmons wrote:
>
>> One worry I have about always leaving process-connection-type t is
>> that, depending on the external system state -- specifically whether
>> or not all ptys are busy -- process-connection-type might not have any
>> effect, and the underlying process will rarely (and silently AFAICT)
>> operate in pipe mode.  By forcing process-connection-type nil, one is
>> always testing in the same known mode.
>
> I don't really understand why pty mode is better here than pipe mode.
> Do we need job control, or escape sequences, or anything else specific
> to pty?  If we use pty, won't these features, on the contrary, get in
> the way somewhere?  We have to respond to only one prompt from
> ldapsearch, and for that pipes should work well.  It's not like when the
> user is interacting with the process (like in shell mode).  The user may
> be unaware that external process is at all invoked.

Yes, my inclination was (and is) to use pipe mode, but I don't know
enough about the implications of either mode, in practice, to
definitively say.

I searched the mailing list archives and current Emacs source code for
process-connection-type and there are many different places where it is
set to either t or nil to accommodate some specific operating system or
program.  Some of these might even have been workarounds for the Darwin
process.c bug you seem to have fixed properly.  Others, like the one I
saw for Solaris, would be harder to test.

Unless we can prove that pty mode can fail against ldapsearch -W in some
semi-realistic valid use case -- maybe a password with a weird
character, or a really really long password that gets buffered or
something -- or conversely that pipe mode can fail in some case where
pty mode would succeed, then I feel I have to defer to Eli's experience
and recommendation.  I need to at least give him a chance to update the
documentation; hopefully that will clarify when to use each mode
definitively.

(I also wonder what happens to a process (e.g., Bash) that needs a pty,
when all ptys are busy.  I haven't done any experiments to check.  At
least in one case I saw in the source code, ange-ftp.el, the program
would just hang in pipe mode.  In that case, it seems like there should
be a way to tell start-process to signal an error if it doesn't get the
desired pty.)

> Your suggestion to condionally fix this for Darwin on release looks
> good.

OK, can you try the attached patch before I push it to emacs-26?

> As for 33154, I'll live with it for some more days, and wait for more
> people to look at it.  And if all goes well, then I'll push it next
> week.

OK, sounds good.

Thanks,
Thomas


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-LDAP-Set-process-connection-type-to-t-on-Darwin.patch --]
[-- Type: text/x-diff, Size: 1263 bytes --]

From ce820679ecc4585dd6ab401f8f027e0e527b6092 Mon Sep 17 00:00:00 2001
From: Thomas Fitzsimmons <fitzsim@fitzsim.org>
Date: Fri, 26 Oct 2018 16:53:19 -0400
Subject: [PATCH] LDAP: Set process-connection-type to t on Darwin

* lisp/net/ldap.el (ldap-search-internal): Set
process-connection-type to t on Darwin.  Do not merge to
master.  (Bug#33050)
---
 lisp/net/ldap.el | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/lisp/net/ldap.el b/lisp/net/ldap.el
index 7b47a54b9f..b106de02e9 100644
--- a/lisp/net/ldap.el
+++ b/lisp/net/ldap.el
@@ -646,7 +646,12 @@ ldap-search-internal
 	       (not (equal "" sizelimit)))
 	  (setq arglist (nconc arglist (list (format "-z%s" sizelimit)))))
       (if passwd
-	  (let* ((process-connection-type nil)
+	  ;; Work around Bug#33154, see also Bug#33050.  Leaving
+	  ;; process-connection-type at its default (typically t)
+	  ;; would probably be fine too, however this is the minimal
+	  ;; change on the release branch that fixes ldap.el on Darwin
+	  ;; and leaves other operating systems unchanged.
+	  (let* ((process-connection-type (eq system-type 'darwin))
 		 (proc-args (append arglist ldap-ldapsearch-args
 				    filter))
 		 (proc (apply #'start-process "ldapsearch" buf
-- 
2.11.0