From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Matthias Dahl Newsgroups: gmane.emacs.devel Subject: Re: wait_reading_process_ouput hangs in certain cases (w/ patches) Date: Mon, 6 Nov 2017 15:15:48 +0100 Message-ID: <5150d198-8dd3-9cf4-5914-b7e945294452@binary-island.eu> References: <83lgjz8eiy.fsf@gnu.org> <831slp98ut.fsf@gnu.org> <83tvyj62qg.fsf@gnu.org> <83r2tetf90.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1509977770 12516 195.159.176.226 (6 Nov 2017 14:16:10 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 6 Nov 2017 14:16:10 +0000 (UTC) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 Cc: emacs-devel@gnu.org To: Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Nov 06 15:16:01 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1eBiC1-0002oj-AY for ged-emacs-devel@m.gmane.org; Mon, 06 Nov 2017 15:16:01 +0100 Original-Received: from localhost ([::1]:48414 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eBiC5-0003o0-KG for ged-emacs-devel@m.gmane.org; Mon, 06 Nov 2017 09:16:05 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:58018) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eBiBy-0003nj-IT for emacs-devel@gnu.org; Mon, 06 Nov 2017 09:15:59 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eBiBs-0002ug-Mj for emacs-devel@gnu.org; Mon, 06 Nov 2017 09:15:58 -0500 Original-Received: from ud19.udmedia.de ([194.117.254.59]:38060 helo=mail.ud19.udmedia.de) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eBiBs-0002u9-D7 for emacs-devel@gnu.org; Mon, 06 Nov 2017 09:15:52 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=simple; d=binary-island.eu; h= subject:to:cc:references:from:message-id:date:mime-version :in-reply-to:content-type:content-transfer-encoding; s=k1; bh=dM YLp8iRBK1YedojkuTqLSFG5OcTF2Tnwpnr6EHirQ0=; b=pk61zcWRMY/tp6LEgw k8lrL6wHdPvke+ARMXo8b59rB1AIqgcyckV5UE4GUKMbUGxtmbcgPPm2Q+0Cai5A wiD+9cbqBWsFUzyChck8Pfcp4AM5ynyesOyefSBuh+xB3o1rgXXRNKNLCCof1m6z xDAFr8c7osX/8n9xEJEhoqwss= Original-Received: (qmail 7377 invoked from network); 6 Nov 2017 15:15:48 +0100 Original-Received: from unknown (HELO ?IPv6:2a02:810b:c540:234:36aa:25b9:ca8f:d05f?) (ud19?126p1@2a02:810b:c540:234:36aa:25b9:ca8f:d05f) by mail.ud19.udmedia.de with ESMTPSA (ECDHE-RSA-AES128-GCM-SHA256 encrypted, authenticated); 6 Nov 2017 15:15:48 +0100 Openpgp: id=1E87ADA02EFE759EFC20B2D1042F47D273AA780C In-Reply-To: <83r2tetf90.fsf@gnu.org> Content-Language: en-US X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 194.117.254.59 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:219946 Archived-At: Hello Eli... On 04/11/17 13:11, Eli Zaretskii wrote: > OK. I think I'm okay with your patches, but IMO they need a minor > improvement: instead of setting got_some_output to an arbitrary value > of 1, the code should set it to the increment of the bytes read from > the sub-process. This is what we do when we actually read from the > process, and I think this scenario should behave the same. WDYT? I know what you mean -- and even though this was bugging me as well when I implemented it, it is actually on purpose. First of all, I wanted to make as few assumptions as possible about how processes are used and be on the conservative/defensive side with this solution, to make sure not to introduce new bugs or corner-cases. In this particular case, I wanted to minimize any integer overflow problems. Granted, depending on how many bits unsigned long has, it is very (!) unlikely to cause any issues, but otoh that is usually how bugs get introduced in the first place. So if I had calculated the difference to set got_some_output properly, I would have had to take the integer overflow problematic into account. The current solution has exactly one corner-case which is, imho, extremely unlikely: You would have to run wait_reading_process_output for a full wrap-around to exactly the same byte count that it was started with in the first place. It is safe to assume that will practically really never happen. Also, no user of wait_reading_process_output uses the return value in such a way at all -- which I would also consider a bug since it is not what is officially documented for the return value in the first place. It simply says positive, negative or zero... without stating that it will actually return the bytes read. So, imho, I would lean toward the current solution since it is as simple as possible with as few corner-cases as possible. If you would like to keep the status quo though and to always return how many bytes were actually read, then I suggest a different approach altogether. Instead of keeping track how many bytes were read for the total lifetime of a process, we could track only how many bytes were read for an in-flight wait. The would require a byte counter and a counter for how many waits are currently in-flight, so the last one (or first one) could zero the byte counter again. That would make things quite a bit more complicated, imho... and without having a proper gain to show for it. > With that change, it's okay to commit this to the master branch. May I suggest, even though it is rather late, to also consider this for the emacs-26 branch? Since it is not a new feature but a bug fix to a problem people are actually running into (see Magit), I think it would be justified. Personally, I have run a patched emacs-26 all the time here without any problems. Thanks again for taking the time. So long, Matthias -- Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu