From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: joaotavora@gmail.com (=?utf-8?B?Sm/Do28gVMOhdm9yYQ==?=) Newsgroups: gmane.emacs.devel Subject: Re: New jrpc.el JSONRPC library Date: Sun, 20 May 2018 16:43:59 +0100 Message-ID: <87d0xqs56o.fsf@gmail.com> References: <87lgcrqgvz.fsf@gmail.com> <831seipvdb.fsf@gnu.org> <871se9lyid.fsf_-_@gmail.com> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Trace: blaine.gmane.org 1526830934 19459 195.159.176.226 (20 May 2018 15:42:14 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sun, 20 May 2018 15:42:14 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) Cc: Eli Zaretskii , monnier@iro.umontreal.ca, vibhavp@gmail.com, emacs-devel@gnu.org To: Philipp Stephani Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Sun May 20 17:42:09 2018 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 1fKQTI-0004t3-JP for ged-emacs-devel@m.gmane.org; Sun, 20 May 2018 17:42:08 +0200 Original-Received: from localhost ([::1]:46869 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fKQVN-0001Om-Qj for ged-emacs-devel@m.gmane.org; Sun, 20 May 2018 11:44:17 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:48708) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fKQVD-0001Of-Jm for emacs-devel@gnu.org; Sun, 20 May 2018 11:44:09 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fKQVA-0005NJ-CG for emacs-devel@gnu.org; Sun, 20 May 2018 11:44:07 -0400 Original-Received: from mail-wr0-x234.google.com ([2a00:1450:400c:c0c::234]:45646) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fKQVA-0005N6-22; Sun, 20 May 2018 11:44:04 -0400 Original-Received: by mail-wr0-x234.google.com with SMTP id w3-v6so5909029wrl.12; Sun, 20 May 2018 08:44:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version:content-transfer-encoding; bh=CdfCf3Rto48PeTEFXESAQb1ezLROVKpJesJqtkprhIc=; b=VbHxDp1jMlZy3gqTiZrTmvKOBHS8pvxMMGD0MAQv0OkVZPHqdZMAEh01SyGCzAfvG8 pmMwuSWSP1mrR5waqQpwhZK+xpZ8CY+lLGgyU1M3CCJOWhq2g9iFEKfcyFpjT1IJdIdJ 6dQVmpg2IXPV3bKHs1DGgnR3DrezLt0vCbkQITsEAxCHe0oeGY/BlleA+YaOmoUliLxX jYNLNIrLfbJ/fWJ5rQYGe9Vf3iJEtREK45l9IoMv0VZTM3sc3xGn3FDlkfkhyd7zd+yS d80Dz0TluyQ0f0ko1G4M8BwF5EHWqPN+gr5fWOZbUQrholnQaKWGK+Tl4ZXWf51t6qG0 /khQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=CdfCf3Rto48PeTEFXESAQb1ezLROVKpJesJqtkprhIc=; b=K6m6H1guF0TEy63YMpu0J73QHOymf02gNT+hd90NZ+dXkMkjQrYimobCRgH5xoChIf 0A5TzfegM5lrTb6J5tQ5doHf16unQN1fF11kdZXf7w68f6pakg7jRI9XCWxRuDwODVXT uJrAk8gyBD+a8esvhRxJvUYnTAKL+mY2NOb9aA7AbwM40OOi4nyDH5RhzMD83qiw6nnL bujt1k+igR1A3aEiu12Duc2CjRwX33JPRCw1pmbc2qE8lCkEagtjt2PSSDH9j3aF6Tj4 QupXHD5L1DLL3Gd/tvz52wE1mKwbv7igLdOtzhl/AF7uut4Gi8rBeQNyUo3tbl0z4g7K RZow== X-Gm-Message-State: ALKqPwenjs2KdvVZ4SmyzSVjICQouNovKZXMN4eHDVUDx0mXQ2VAsSsd EMGpivTLvbRx7kK4314gQHoPskWb X-Google-Smtp-Source: AB8JxZqgj83fDvMoe57QqqSOkQX1BzFZJFAqIA+mltqZOHMwozlUDjxoYdoafH72Na8HuiOZUZP/Dw== X-Received: by 2002:adf:db4f:: with SMTP id f15-v6mr14714366wrj.212.1526831042379; Sun, 20 May 2018 08:44:02 -0700 (PDT) Original-Received: from lolita.yourcompany.com (188.139.62.94.rev.vodafone.pt. [94.62.139.188]) by smtp.gmail.com with ESMTPSA id 123-v6sm15054731wmt.19.2018.05.20.08.44.00 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sun, 20 May 2018 08:44:01 -0700 (PDT) In-Reply-To: (Philipp Stephani's message of "Sat, 19 May 2018 13:34:45 +0200") X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2a00:1450:400c:c0c::234 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:225476 Archived-At: Philipp Stephani writes: > Jo=C3=A3o T=C3=A1vora schrieb am Fr., 18. Mai 2018= um 18:28 Uhr: > >> I propose this library, jrpc.el, for Emacs core (or GNU ELPA, or > Thanks. I think that separating out JSONRPC from LSP and having a > JSONRPC library in core are highly valuable additions that I'd > strongly support. Some review comments follow; sorry that there are > so many, but I think as a foundational library jrpc.el should be as > high-quality as possible: Apology *not accepted* :-). Having reviewers provide so many insightful comments is a previlige and a pleasure. The high standards of the Emacs project are one of the reasons I like to contribute to it. I'll triaged your comments into "No", "Maybe", "Ok". The former require some more convincing on your part :-) I've already applied some of your suggestions (and some that Stefan sent off-list, too). See the jsonrpc-refactor branch of Eglot's github repository. The latest version of jsonrpc.el (I changed the name) lives in: https://raw.githubusercontent.com/joaotavora/eglot/jsonrpc-refactor/jsonrpc= .el > - Please add extensive unit and integration tests. There are lots of > corner cases (what happens when the server times out, sends an invalid > or incomplete response, sends responses in the wrong order, etc.) that > should all be tested. OK. It's in the plans, if not in the stars. Help appreciated. :-) We can convert some of those examples to tests. Eglot's jsonrpc-refactor branch, where I'm currently developing jsonrpc.el, already has integration tests, so at least a part of jsonrpc.el is already covered. > - Please remove all defcustoms and defvars. jrpc.el is a low-level > library, it doesn't make sense for it to maintain global state or be > globally customizable. Rather, the timeout could either be a mandatory > argument, or a reasonable constant default could be selected, and the > other variables should be per-process. OK. Makes sense. Done. > - Also please remove `jrpc-current-process'. There is no general > notion of a "current" process; Maybe. Not convinced. It's opininated, sure. It supposes that just most JSONRPC applications would have the notion of a "session". So jsonrpc.el makes this suggestion to its users, allowing them to configure what a session means for them. They would also get access to M-x jsonrpc-events-buffer. This is entirely optional on the client's part, but I see no harm in JSONRPC providing this convenience. Perhaps the name is off and should mention "session". > - Please don't use the `error' symbol directly. Instead, define your > own error symbols for each error category using `define-error'. Only > then clients have the chance to catch and handle errors. OK. Done. Though technically they can already catch and handle them. What they can't do is catch *only* JSONRPC errors and let the others thru. I happen to think that's a bad idea because a JSONRPC error is an error in your program. But there's no reason to deny this, too. > - `jrpc-define-process-var' isn't JSONRPC-specific, it should be moved > somewhere else (subr.el)? Yes, but if we do this it'll be harder for clients like eglot.el to keep supporting Emacs 26.1 (assuming jsonrpc.el would be in GNU ELPA and master simultaneously). > - Instead of defining process variables, consider using a structure or > EIEIO class to encapsulate all needed state for the process client. > Maybe. Structures is a bad idea, but defclass isn't at all. > - Please make it possible to run servers remotely. Unfortunately this > isn't possible with `make-process', so you have to use > `start-file-process' instead if `default-directory' is remote. As an > alternative to using `default-directory', consider passing a > REMOTE-DIRECTORY argument to `jrpc-connect' so that the behavior > doesn't depend on ambient context. Maybe. I don't fully understand the example. But jsonrpc-connect allows you= to pass any pre-connected process object. It will replace its buffer, sentinel and filter. In the "autostart-server" branch of eglot.el, for example, there is a function to I start a local process with special arguments to that it open a server, then connect to it, then return that connected process. This is for Windows LSP servers that can't to stdio comms. > - I think many of the names can be internal (i.e. contain a double > dash) unless they are explicitly part of the public API. Maybe. What names? The names that I want to make public are already public, and all the others aren't. > - Probably jrpc-async-request should just return nil. Or is there a > situation where callers have to know about IDs or timers? I think > those should be treated as implementation details and not be exposed > to clients. OK. Makes sense. > - Please use hashtables or alists instead of plists. There are many > more functions to operate on hashtables and alists, Maybe. Hmm, I really really don't know about this. Have you tried cl-destructuring-bind with plists? It really is the SH&T for JSON. > and json.c doesn't support plists. That's the superior argument. I didn't know about json.c until very recently. I'll play along, but can you help me rewrite jsonrpc-lambda to something less consing perhaps? (cl-defmacro jsonrpc-lambda (cl-lambda-list &body body) (declare (indent 1) (debug (sexp &rest form))) (let ((jsonobj (gensym "jsonrpc-lambda-elem"))) `(lambda (,jsonobj) (apply (cl-function (lambda ,cl-lambda-list ,@body)) (let (plist) (maphash (lambda (k v) (push v plist) (push (if (keywordp k) k (intern (format ":%s" k))) plist)) ,jsonobj) plist))))) > - The HTTP-style enveloping (using HTTP headers to separate responses) > is technically not part of JSONRPC. You could either add support for > other enveloping methods (though I'm not aware of any), or at least > call this out in the documentation. OK. Yes, it's most definetly not JSONRPC. It's an LSP thing. This is the reason I need to rework the "process" object into a proper defclass and make generic functions that only apply this enveloping to the proper LSPlike objects (which could/should be the default object types returned by jsonrpc-connect). > - I think `string-bytes' isn't guaranteed to return the number of > UTF-8 bytes. Rather you should encode the string explicitly to a > unibyte string using `encode-coding-string', and then use > `no-conversion' for the process coding systems. > I think Eli has already answered this authoritatively. > - Consider also sending (and parsing) a Content-Type header: > Content-Type: application/vscode-jsonrpc; charset=3Dutf-8. OK. > - I don't quite understand how `jrpc-ready-predicates' is supposed to > be used. Are clients supposed to have some form of server-specific > sidechannel to detect when the server is ready? Yes that's precisely it (though it's entirely optional on the client part). > It might be simpler to solve this problem in Emacs core, e.g. by > adding a timeout to `process-send-string'. No, the "ready" here is for the application, not the transport. I understand it's obscure for a reader, so let me explain and then this might go into the doc. Obviously, this came out of necessity in eglot.el, but I think its good functionality to keep in jsonrpc.el refactor (and it'd be really hard to implement outside it.) The problem arises in the ordering of sending multiple asynchronous JSONRPC requests. So, for example in LSP, it doesn't make sense to send most requests if there are outstanding changes in the buffer that you haven't told the server about. But because the latter can come in through idle timers, it's useful to have this generic :deferred mechanism: you just put in `jsonrpc-ready-predicates' a function that returns nil if there are outstanding changes and then write code without worrying about whether the server is ready in that particular situation. This is better than sprinkling the code with "send-outstanding-changes-just-to-be-sure" before you make requests. RLS (Rust's Language Server) has a more complicated limitation, so eglot.el puts an additional predicate there for rust-mode.el > - I'd call the library jsonrpc.el (or json-rpc.el or jsonrpc2.el > or...) if that name isn't already taken. OK. I don't think it is. Done Jo=C3=A3o