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: Fri, 10 Nov 2017 15:45:30 +0100 Message-ID: <398f8d17-b727-d5d6-4a31-772448c7ca0d@binary-island.eu> 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> <83tvy7s6wi.fsf@gnu.org> <83inemrqid.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------C542DE5F5E0EEADCBB2BEC38" X-Trace: blaine.gmane.org 1510325184 21855 195.159.176.226 (10 Nov 2017 14:46:24 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 10 Nov 2017 14:46:24 +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 Fri Nov 10 15:46:19 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 1eDAZR-0005CN-KO for ged-emacs-devel@m.gmane.org; Fri, 10 Nov 2017 15:46:13 +0100 Original-Received: from localhost ([::1]:42112 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eDAZY-0001Kz-KQ for ged-emacs-devel@m.gmane.org; Fri, 10 Nov 2017 09:46:20 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:40871) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eDAYt-0001Kg-KE for emacs-devel@gnu.org; Fri, 10 Nov 2017 09:45:41 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eDAYq-0000pw-3J for emacs-devel@gnu.org; Fri, 10 Nov 2017 09:45:39 -0500 Original-Received: from ud19.udmedia.de ([194.117.254.59]:53214 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 1eDAYp-0000ip-JE for emacs-devel@gnu.org; Fri, 10 Nov 2017 09:45:36 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=simple; d=binary-island.eu; h=from :subject:to:cc:references:message-id:date:mime-version :in-reply-to:content-type; s=k1; bh=DN8gtPx2PAdJO5nI4zYqZql0ug45 K2VDzn3JEn0ROlk=; b=qwp1ccG1xcbbAQGH30t0TgGjvVKJGNXHNqW6/5b2xMM7 BdsvFZNISmhDYoDYUrhBKVo4kM8HDizU04myAMf90Lyf4OMhK4KBKjmQYIAPyVOv A4y4zPDrNwKQoEW9lqq9JhFKcE/821wLHhHgakvW/vOKKS5K5u4HBrwM5H3pMy0= Original-Received: (qmail 1697 invoked from network); 10 Nov 2017 15:45:32 +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); 10 Nov 2017 15:45:32 +0100 Openpgp: id=1E87ADA02EFE759EFC20B2D1042F47D273AA780C In-Reply-To: <83inemrqid.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:220026 Archived-At: This is a multi-part message in MIME format. --------------C542DE5F5E0EEADCBB2BEC38 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Hello Eli... Attached you will find the revised patches which now properly set the number of bytes read. Sorry for the delay, but real life intervened. Turns out, things were _a lot_ easier than I thought since the C standard actually does have well-defined rules for computation on unsigned types, so that no overflow happens. And since it is modular arithmetic, it perfectly fits our use-case. :-) Sometimes the simplest solution is right in front of your eyes and you are thinking way too complicated. So sorry for the trouble. Had I known this earlier, I would not have skipped that part -- even though I had the best intentions, obviously. On with the rest of your mail... On 07/11/17 17:40, Eli Zaretskii wrote: > If we want, we can make it EMACS_INT, which will give us as much as > Emacs can gobble. Thanks for the suggestion, I changed it to EMACS_UINT -- we need an unsigned type. > I had my > share of writing code based on what I read in the function commentary > and some general common sense and familiarity with the internals, only > to find out later that they were incomplete or even prone to wrong > interpretation. I'd like to minimize such occurrences as much as I > can. I guess I was overly idealistic. Sorry for that. Nevertheless, it would be nice to improve the situation and make sure that future changes to the codebase also take the commentaries into account... and put those themselves to the same high standard as the implementation itself. Just my two cents... > But that's exactly the problem: we _don't_have_ documented interfaces > on the C level, at least not of the quality we would like to have. Do > you really think that the commentary at the beginning of > wait_reading_process_output describes what the function does anywhere > close to completeness? Far from it. I know. OTOH, wait_reading_process_output might be a bad example as it really could benefit greatly from a refactor since it is way too big, complex and unclear. And describing its function properly and concisely could prove quite difficult. But generally, I pretty much agree on all that you said in the rest of your mail. > I'd actually say that the commentary is incomplete because it doesn't > say what that "positive" value means. Returning an opaque value from > a function is not useful, unless you can pass it later to the same > function to get it to do something special. I agree. It also leads the user to make assumptions about the value that might or might not turn out true for all cases... without properly reading the code. Thanks again for all your great feedback and review. I hope we now have something that can be applied to the master branch. If anything comes up in terms of bugs or problems that might be related, please poke me if I miss it on the list and I will (try to) fix it. Have a nice weekend, Matthias -- Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu --------------C542DE5F5E0EEADCBB2BEC38 Content-Type: text/x-patch; name="0001-Add-process-output-read-accounting.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename="0001-Add-process-output-read-accounting.patch" =46rom 76b9697bf53c151849face5f95f2b07c2d0b3511 Mon Sep 17 00:00:00 2001 From: Matthias Dahl Date: Tue, 24 Oct 2017 15:55:53 +0200 Subject: [PATCH 1/2] Add process output read accounting This tracks the bytes read from a process's stdin which is not used anywhere yet but required for follow-up work. * src/process.c (read_process_output): Track bytes read from a process. * src/process.h (struct Lisp_Process): Add infd_num_bytes_read to track bytes read from a process. --- src/process.c | 2 ++ src/process.h | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/process.c b/src/process.c index fc46e74332..904ca60863 100644 --- a/src/process.c +++ b/src/process.c @@ -5900,6 +5900,8 @@ read_process_output (Lisp_Object proc, int channel)= /* Now set NBYTES how many bytes we must decode. */ nbytes +=3D carryover; =20 + p->infd_num_bytes_read +=3D nbytes; + odeactivate =3D Vdeactivate_mark; /* There's no good reason to let process filters change the current buffer, and many callers of accept-process-output, sit-for, and diff --git a/src/process.h b/src/process.h index 5a044f669f..d49be55f10 100644 --- a/src/process.h +++ b/src/process.h @@ -129,6 +129,8 @@ struct Lisp_Process pid_t pid; /* Descriptor by which we read from this process. */ int infd; + /* Byte-count for process output read from `infd'. */ + EMACS_UINT infd_num_bytes_read; /* Descriptor by which we write to this process. */ int outfd; /* Descriptors that were created for this process and that need --=20 2.15.0 --------------C542DE5F5E0EEADCBB2BEC38 Content-Type: text/x-patch; name="0002-src-process.c-wait_reading_process_output-Fix-wait_p.patch" Content-Transfer-Encoding: quoted-printable Content-Disposition: attachment; filename*0="0002-src-process.c-wait_reading_process_output-Fix-wait_p.pa"; filename*1="tch" =46rom a309cc76c93826c6ac7d51bba88900738910e504 Mon Sep 17 00:00:00 2001 From: Matthias Dahl Date: Tue, 24 Oct 2017 15:56:47 +0200 Subject: [PATCH 2/2] * src/process.c (wait_reading_process_output): Fix wait_proc hang. If called recursively (through timers or process filters by the means of accept-process-output), it is possible that the output of wait_proc has already been read by one of those recursive calls, leaving the original call hanging forever if no further output arrives through that fd and no timeout has been specified. Implement proper checks by taking advantage of the process output read accounting. --- src/process.c | 32 +++++++++++++++++++++++++++++++- 1 file changed, 31 insertions(+), 1 deletion(-) diff --git a/src/process.c b/src/process.c index 904ca60863..d4e152eb1c 100644 --- a/src/process.c +++ b/src/process.c @@ -5003,6 +5003,8 @@ wait_reading_process_output (intmax_t time_limit, i= nt nsecs, int read_kbd, struct timespec got_output_end_time =3D invalid_timespec (); enum { MINIMUM =3D -1, TIMEOUT, INFINITY } wait; int got_some_output =3D -1; + EMACS_UINT initial_wait_proc_num_bytes_read =3D (wait_proc) ? + wait_proc->infd_num_byte= s_read : 0; #if defined HAVE_GETADDRINFO_A || defined HAVE_GNUTLS bool retry_for_async; #endif @@ -5161,6 +5163,20 @@ wait_reading_process_output (intmax_t time_limit, = int nsecs, int read_kbd, && requeued_events_pending_p ()) break; =20 + /* Timers could have called `accept-process-output', thus read= ing the output + of wait_proc while we (in the worst case) wait endlessly fo= r it to become + available later. So we need to check if data has been read = and break out + early if that is so since our job has been fulfilled. */ + if (wait_proc + && wait_proc->infd_num_bytes_read !=3D initial_wait_proc_n= um_bytes_read) + { + /* Computations on unsigned types are well defined and won= 't overflow, + so this is safe even if our initial value > our current= value, in + case of a wrap around. (ISO/IEC 9899:1999 =C2=A76.2.5/9= ) */ + got_some_output =3D wait_proc->infd_num_bytes_read + - initial_wait_proc_num_bytes_read; + } + /* This is so a breakpoint can be put here. */ if (!timespec_valid_p (timer_delay)) wait_reading_process_output_1 (); @@ -5606,7 +5622,21 @@ wait_reading_process_output (intmax_t time_limit, = int nsecs, int read_kbd, buffered-ahead character if we have one. */ =20 nread =3D read_process_output (proc, channel); - if ((!wait_proc || wait_proc =3D=3D XPROCESS (proc)) + + /* In case a filter was run that called `accept-process-ou= tput', it is + possible that the output from wait_proc was already rea= d, leaving us + waiting for it endlessly (if no timeout was specified).= Thus, we need + to check if data was already read. */ + if (wait_proc + && wait_proc->infd_num_bytes_read !=3D initial_wait_pr= oc_num_bytes_read) + { + /* Computations on unsigned types are well defined and= won't overflow, + so this is safe even if our initial value > our cur= rent value, in + case of a wrap around. (ISO/IEC 9899:1999 =C2=A76.2= =2E5/9) */ + got_some_output =3D wait_proc->infd_num_bytes_read + - initial_wait_proc_num_bytes_read; + } + else if ((!wait_proc || wait_proc =3D=3D XPROCESS (proc)) && got_some_output < nread) got_some_output =3D nread; if (nread > 0) --=20 2.15.0 --------------C542DE5F5E0EEADCBB2BEC38--