From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: =?UTF-8?Q?Jo=C3=A3o_?= =?UTF-8?Q?T=C3=A1vora?= Newsgroups: gmane.emacs.bugs Subject: bug#60088: 29.0.60; Eglot loses messages from gopls Date: Fri, 16 Dec 2022 08:56:56 +0000 Message-ID: References: <87pmckg9sw.fsf@gmail.com> Mime-Version: 1.0 Content-Type: multipart/alternative; boundary="0000000000006f591e05efee22de" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="8574"; mail-complaints-to="usenet@ciao.gmane.io" To: 60088@debbugs.gnu.org, eliz@gnu.org Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Fri Dec 16 09:56:23 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1p66Vy-0001za-03 for geb-bug-gnu-emacs@m.gmane-mx.org; Fri, 16 Dec 2022 09:56:22 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1p66Vg-000335-CO; Fri, 16 Dec 2022 03:56:04 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p66Ve-00032e-3b for bug-gnu-emacs@gnu.org; Fri, 16 Dec 2022 03:56:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1p66Vd-0006zE-RU for bug-gnu-emacs@gnu.org; Fri, 16 Dec 2022 03:56:01 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1p66Vd-000692-Nn for bug-gnu-emacs@gnu.org; Fri, 16 Dec 2022 03:56:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: =?UTF-8?Q?Jo=C3=A3o_?= =?UTF-8?Q?T=C3=A1vora?= Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 16 Dec 2022 08:56:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 60088 X-GNU-PR-Package: emacs Original-Received: via spool by 60088-submit@debbugs.gnu.org id=B60088.167118095123612 (code B ref 60088); Fri, 16 Dec 2022 08:56:01 +0000 Original-Received: (at 60088) by debbugs.gnu.org; 16 Dec 2022 08:55:51 +0000 Original-Received: from localhost ([127.0.0.1]:48050 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1p66VS-00068m-Gi for submit@debbugs.gnu.org; Fri, 16 Dec 2022 03:55:51 -0500 Original-Received: from mail-oa1-f45.google.com ([209.85.160.45]:41732) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1p66VQ-00068W-HY for 60088@debbugs.gnu.org; Fri, 16 Dec 2022 03:55:49 -0500 Original-Received: by mail-oa1-f45.google.com with SMTP id 586e51a60fabf-1441d7d40c6so2481617fac.8 for <60088@debbugs.gnu.org>; Fri, 16 Dec 2022 00:55:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :from:to:cc:subject:date:message-id:reply-to; bh=u5XldW6zxlYwjfmRtReLZpZefnwWRE+72qctSIml2hw=; b=WGDjPeSK5HAhqmX7Gdr9AbALHbAwpzasx/h4WCaRp+O5p1kWmGt8S2hjIoKJTv603k PUM2doHZkSVRHaCrieRMl4T58wHfvTVsCJZtgi6FdfIhak9xM6+KpBCw4JnmXeFU617x 7461MxON9j2PkZlA4qcFiT/KQ9/XuNlz9VsEl/Feg2x4VaxLA/oZbCO2wbBplkCcq2eb pVgJqQJoRrHZFYSsV30qE5RdWjgvgQKyD1H6UbWAVG81TtaLMCVgAKzMze7craubZa7u wB1hCodtMZRD7Zj/FUzDmCZtRwg3IlmxhJFbSHdPzfTfexOLnAZeTYVisHUJCoUCYOQV jTmQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=u5XldW6zxlYwjfmRtReLZpZefnwWRE+72qctSIml2hw=; b=dfP3OG6MtzAEMqbWywxo4tVQ86r8ux3fAhE42KIlcf4fZn3CbFKhUf7Cx4MlR8UuF8 7RawGj9+DuYve8LYA8YdAfCd+xTrOut6ziUVBkHBhOptFyKHgjBjx750yg2wV+J3UIXz 25pAq4eIiYjNOULuHeaUWiz01mA4S+FWYYTImu10oMT77PUsbiwdC1WwAaMjhpKRJAG3 g6wz1fblSYfoV+ZybW4UFluZnSltcgBasTNNL+C+sEIgLo78bO7J0HDI8NveXgQUIiUG Ik/Jssa9lShpFnqFDz6JYjfci804tDR3iuNjxOBf6kM2oP2EM3LJb+eUZ+JYw8wqVZvS mWqA== X-Gm-Message-State: AFqh2kr/T0Pb/OMpoa2vNE64abxr/g1oEY5qAb2dOx8d5xS8lX3uTRB9 R5pXToPyS03tHsMGVh9GrE0R2VtkaDw4VYxD3ub8sw2t X-Google-Smtp-Source: AMrXdXsuGcrzc9FAkVgoJ1zwCsDQC4ERHrYkA8CTEGVJs1655bwIrWI26UTf2XzLRq1dqyACbyZDzlQdqDTRv834tII= X-Received: by 2002:a05:6870:3116:b0:143:7889:c525 with SMTP id v22-20020a056870311600b001437889c525mr363266oaa.171.1671180942529; Fri, 16 Dec 2022 00:55:42 -0800 (PST) In-Reply-To: <87pmckg9sw.fsf@gmail.com> X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:251195 Archived-At: --0000000000006f591e05efee22de Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable I've now pushed the patch/bugfix to emacs-29 and bumped version of jsonrpc.el so we should get a new GNU ELPA package soon. GNU ELPA Eglot users should wait for the package to be bumped too. Jo=C3=A3o On Thu, Dec 15, 2022 at 11:55 AM Jo=C3=A3o T=C3=A1vora wrote: > [Eli, I have CC'ed you directly because this bug concerns a technique to > avoid recursive process filters, which seems like an area you are expert > in.] > > Hi, > > In some situations, Eglot is losing some messages from the Go language > server, 'gopls'. The problem has been reproduced and identified. > > For context, this started in: > > * https://github.com/joaotavora/eglot/issues/587#issuecomment-1221072833 > * https://github.com/golang/go/issues/54559 > > (In the former link, there are reports of what seem to be other > problems. The reports are missing clear reproduction recipes, so they > may not be the same problem I'm about to describe.) > > Reproduction: > > Ensure go-mode is installed from https://melpa.org/#/go-mode > $ go install golang.org/x/tools/gopls@latest > $ git clone git clone https://github.com/google/nftables > $ emacs -Q -f package-initialize nftables/nftables_test.go > M-x eglot > > The last step will connect to the server, exchange some messages. It > will not block Emacs, but the Eglot session will be completely > non-functional. > > Analysis: > > Many essential JSONRPC messages have not been exchanged, they have been > lost. The bytes of the lost messages from the gopls side can be seen in > the process buffer (M-x list-processes, then click the link to the > hidden process buffer). > > The problem happens in jsonrpc.el's jsonrpc--process-filter. The code > does not expect this function to be called recursively, but that can > happen if the client code invoked by its jsonrpc--connection-receive > callee eventually sends a follow-up message to the server. This is a > common scenario, especially during LSP initialiation. It has been > working fine for a number of years and a large number of servers. > > However: > > * if that follow-up message is large enough (this is why the largish > nftables_test.go file is needed) AND > > * the server has already sent us some output with just the right (wrong) > timing. > > then the process-send-string call that is eventually reached will in > fact cause more output to arrive from the process and a recursive > invocation of the process filter. > > Resolution: > > I've looked at a number of ways to solve this problem, from avoiding the > follow up message in Eglot's side to different techniques on the jsonrpc > side. None of them are as satisfactory as the patch after my sig, which > simply re-schedules the would-be recursive call to re-run from within a > timer stack as soon as possible. > > The only problem I can envision with this approach would be if multiple > recursive calls are launched from the same jsonrpc--connection-receive > stack frame and would somehow not be executed in the correct order. > That doesn't seem to be a problem after some experiments like > > (progn > (run-at-time 0 nil #'insert "a") > (run-at-time 0 nil #'insert "b") > (run-at-time 0 nil #'insert "c")) > > which inserts the predicatbly sane "abc". If this in-order execution is > somehow not guaranteed, there is the alternate more complex technique of > inserting the output into the process buffer immediately, and schedule a > single processing call thereafter. > > I will install the following patch soon unless someone objects to this > approach. > > Jo=C3=A3o > > diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el > index 90833e1c1d..76bbf28138 100644 > --- a/lisp/jsonrpc.el > +++ b/lisp/jsonrpc.el > @@ -548,11 +548,27 @@ jsonrpc--process-sentinel > (delete-process proc) > (funcall (jsonrpc--on-shutdown connection) connection))))) > > -(defun jsonrpc--process-filter (proc string) > +(defvar jsonrpc--in-process-filter nil > + "Non-nil if inside `jsonrpc--process-filter'.") > + > +(cl-defun jsonrpc--process-filter (proc string) > "Called when new data STRING has arrived for PROC." > + (when jsonrpc--in-process-filter > + ;; Problematic recursive process filters may happen if > + ;; `jsonrpc--connection-receive', called by us, eventually calls > + ;; client code which calls `process-send-string' (which see) to, > + ;; say send a follow-up message. If that happens to writes enough > + ;; bytes for additional output to be received, we have a problem. > + ;; > + ;; In that case, remove recursiveness by re-scheduling ourselves to > + ;; run from within a timer as soon as possible. > + > + (run-at-time 0 nil #'jsonrpc--process-filter proc string) > + (cl-return-from jsonrpc--process-filter)) > (when (buffer-live-p (process-buffer proc)) > (with-current-buffer (process-buffer proc) > (let* ((inhibit-read-only t) > + (jsonrpc--in-process-filter t) > (connection (process-get proc 'jsonrpc-connection)) > (expected-bytes (jsonrpc--expected-bytes connection))) > ;; Insert the text, advancing the process marker. > > > > > > > > > > > > > --=20 Jo=C3=A3o T=C3=A1vora --0000000000006f591e05efee22de Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
I've now pushed the patch/bugfix to emacs-29 and = bumped
version of jsonrpc.el so we should get a new GNU ELPA pack= age
soon.
=C2=A0
GNU ELPA Eglot user= s should wait for the package to be bumped
too.
Jo=C3=A3o

On Thu, Dec 15, 2022 at 11:55 AM Jo=C3=A3o = T=C3=A1vora <joaotavora@gmail.co= m> wrote:
[Eli, I have CC'ed you directly because this bug concerns a technique = to
avoid recursive process filters, which seems like an area you are expert in.]

Hi,

In some situations, Eglot is losing some messages from the Go language
server, 'gopls'.=C2=A0 The problem has been reproduced and identifi= ed.

For context, this started in:

* https://github.com/joaotavor= a/eglot/issues/587#issuecomment-1221072833
* https://github.com/golang/go/issues/54559

(In the former link, there are reports of what seem to be other
problems.=C2=A0 The reports are missing clear reproduction recipes, so they=
may not be the same problem I'm about to describe.)

Reproduction:

=C2=A0 Ensure go-mode is installed from https://melpa.org/#/go-mode =C2=A0 $ go install golang.org/x/tools/gopls@latest
=C2=A0 $ git clone git clone https://github.com/google/nftables
=C2=A0 $ emacs -Q -f package-initialize nftables/nftables_test.go
=C2=A0 M-x eglot

The last step will connect to the server, exchange some messages.=C2=A0 It<= br> will not block Emacs, but the Eglot session will be completely
non-functional.

Analysis:

Many essential JSONRPC messages have not been exchanged, they have been
lost.=C2=A0 The bytes of the lost messages from the gopls side can be seen = in
the process buffer (M-x list-processes, then click the link to the
hidden process buffer).

The problem happens in jsonrpc.el's jsonrpc--process-filter.=C2=A0 The = code
does not expect this function to be called recursively, but that can
happen if the client code invoked by its jsonrpc--connection-receive
callee eventually sends a follow-up message to the server.=C2=A0 This is a<= br> common scenario, especially during LSP initialiation.=C2=A0 It has been
working fine for a number of years and a large number of servers.

However:

* if that follow-up message is large enough (this is why the largish
=C2=A0 nftables_test.go file is needed) AND

* the server has already sent us some output with just the right (wrong) =C2=A0 timing.

then the process-send-string call that is eventually reached will in
fact cause more output to arrive from the process and a recursive
invocation of the process filter.

Resolution:

I've looked at a number of ways to solve this problem, from avoiding th= e
follow up message in Eglot's side to different techniques on the jsonrp= c
side.=C2=A0 None of them are as satisfactory as the patch after my sig, whi= ch
simply re-schedules the would-be recursive call to re-run from within a
timer stack as soon as possible.

The only problem I can envision with this approach would be if multiple
recursive calls are launched from the same jsonrpc--connection-receive
stack frame and would somehow not be executed in the correct order.
That doesn't seem to be a problem after some experiments like

(progn
=C2=A0 (run-at-time 0 nil #'insert "a")
=C2=A0 (run-at-time 0 nil #'insert "b")
=C2=A0 (run-at-time 0 nil #'insert "c"))

which inserts the predicatbly sane "abc".=C2=A0 If this in-order = execution is
somehow not guaranteed, there is the alternate more complex technique of inserting the output into the process buffer immediately, and schedule a single processing call thereafter.

I will install the following patch soon unless someone objects to this
approach.

Jo=C3=A3o

diff --git a/lisp/jsonrpc.el b/lisp/jsonrpc.el
index 90833e1c1d..76bbf28138 100644
--- a/lisp/jsonrpc.el
+++ b/lisp/jsonrpc.el
@@ -548,11 +548,27 @@ jsonrpc--process-sentinel
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(delete-process proc)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(funcall (jsonrpc--on-shutdown connection= ) connection)))))

-(defun jsonrpc--process-filter (proc string)
+(defvar jsonrpc--in-process-filter nil
+=C2=A0 "Non-nil if inside `jsonrpc--process-filter'.")
+
+(cl-defun jsonrpc--process-filter (proc string)
=C2=A0 =C2=A0"Called when new data STRING has arrived for PROC."<= br> +=C2=A0 (when jsonrpc--in-process-filter
+=C2=A0 =C2=A0 ;; Problematic recursive process filters may happen if
+=C2=A0 =C2=A0 ;; `jsonrpc--connection-receive', called by us, eventual= ly calls
+=C2=A0 =C2=A0 ;; client code which calls `process-send-string' (which = see) to,
+=C2=A0 =C2=A0 ;; say send a follow-up message.=C2=A0 If that happens to wr= ites enough
+=C2=A0 =C2=A0 ;; bytes for additional output to be received, we have a pro= blem.
+=C2=A0 =C2=A0 ;;
+=C2=A0 =C2=A0 ;; In that case, remove recursiveness by re-scheduling ourse= lves to
+=C2=A0 =C2=A0 ;; run from within a timer as soon as possible.
+
+=C2=A0 =C2=A0 (run-at-time 0 nil #'jsonrpc--process-filter proc string= )
+=C2=A0 =C2=A0 (cl-return-from jsonrpc--process-filter))
=C2=A0 =C2=A0(when (buffer-live-p (process-buffer proc))
=C2=A0 =C2=A0 =C2=A0(with-current-buffer (process-buffer proc)
=C2=A0 =C2=A0 =C2=A0 =C2=A0(let* ((inhibit-read-only t)
+=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0(jsonrpc--in-process-filte= r t)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (connection (process-get p= roc 'jsonrpc-connection))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 (expected-bytes (jsonrpc--= expected-bytes connection)))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0;; Insert the text, advancing the process= marker.














--
Jo=C3=A3o T=C3=A1vora
--0000000000006f591e05efee22de--