From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.devel Subject: Re: wait_reading_process_ouput hangs in certain cases (w/ patches) Date: Mon, 06 Nov 2017 18:34:21 +0200 Message-ID: <83tvy7s6wi.fsf@gnu.org> References: <83lgjz8eiy.fsf@gnu.org> <831slp98ut.fsf@gnu.org> <83tvyj62qg.fsf@gnu.org> <83r2tetf90.fsf@gnu.org> <5150d198-8dd3-9cf4-5914-b7e945294452@binary-island.eu> Reply-To: Eli Zaretskii NNTP-Posting-Host: blaine.gmane.org X-Trace: blaine.gmane.org 1509986116 2169 195.159.176.226 (6 Nov 2017 16:35:16 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Mon, 6 Nov 2017 16:35:16 +0000 (UTC) Cc: emacs-devel@gnu.org To: Matthias Dahl Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Mon Nov 06 17:35:10 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 1eBkMf-0000JC-Q7 for ged-emacs-devel@m.gmane.org; Mon, 06 Nov 2017 17:35:09 +0100 Original-Received: from localhost ([::1]:49075 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eBkMn-0003GB-5D for ged-emacs-devel@m.gmane.org; Mon, 06 Nov 2017 11:35:17 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:53361) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eBkLt-0003Eu-6b for emacs-devel@gnu.org; Mon, 06 Nov 2017 11:34:22 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eBkLn-0000TN-Bh for emacs-devel@gnu.org; Mon, 06 Nov 2017 11:34:21 -0500 Original-Received: from fencepost.gnu.org ([2001:4830:134:3::e]:43115) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eBkLn-0000TH-8k; Mon, 06 Nov 2017 11:34:15 -0500 Original-Received: from [176.228.60.248] (port=2961 helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1eBkLm-0008He-Lj; Mon, 06 Nov 2017 11:34:15 -0500 In-reply-to: <5150d198-8dd3-9cf4-5914-b7e945294452@binary-island.eu> (message from Matthias Dahl on Mon, 6 Nov 2017 15:15:48 +0100) X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 2001:4830:134:3::e 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:219951 Archived-At: > Cc: emacs-devel@gnu.org > From: Matthias Dahl > Date: Mon, 6 Nov 2017 15:15:48 +0100 > > 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. Sorry, I don't understand. When we call emacs_read in other cases in that function, the result is an ssize_t, a signed type of the same width as size_t. So why would you need to handle overflow, when reading via emacs_read doesn't? And even if the result could overflow, we have the likes of INT_ADD_WRAPV to make handling of that easier and safer. So I don't see why we should give up in this case. > 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. I don't agree with this line of reasoning. We are not developing a library whose users will be familiar only with the formal interface definition and nothing else. We are talking about code that is being read, studied, and used by Emacs hack^H^H^H^Hdevelopers all the time. Having a non-trivial function whose behavior is hard to describe and remember accurately makes using it error-prone, especially if the deviant behavior happens only in some corner use case that is hard to reproduce. In many cases, the information about these corner cases is lost soon after the code is introduced, and since we have no formal requirements for what functions like this one should do, and no good coverage by the test suite, future changes are impossible to test reliably, in order to make sure they don't break in those corner cases. So the situation where "the function FOO returns the number of bytes read from the subprocess, except when this and that happens, in which case it returns just 1" -- such situation should be avoided at all costs, IME. And in this case, the cost is actually quite low, unless I'm missing something. > 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. Sorry, you lost me half-way through this description. I actually meant to return only the increment of how many bytes were read since the last call, but I don't see why you'd need more than one counter that will be reset to zero once its value is consumed. > > 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? It's a bug that has been there "forever", and the fix is too risky to have it in the middle of a pretest. I don't think we understand well enough all of its implications, and reading from subprocesses is a very central and very delicate part of Emacs. Sorry. Thanks for working on this.