* jsonrpc.el uses "Content-Length" headers which seem specific to LSP @ 2023-10-18 23:17 Adam Porter 2023-10-19 0:11 ` João Távora 0 siblings, 1 reply; 6+ messages in thread From: Adam Porter @ 2023-10-18 23:17 UTC (permalink / raw) To: João Távora; +Cc: emacs-devel Hi João, et al, In our work on hyperdrive.el (a new project currently packaged on NonGNU ELPA), we're beginning a refactor to use a JSON-RPC backend rather than the HTTP-based API we've been using. In the process we've discovered that jsonrpc.el seems to hard-code the use of HTTP-style "Content-Length" headers. These are required by LSP (obviously, one of the major use-cases for jsonrpc.el), however, as best we can tell, JSON-RPC itself does not mention the use of these headers.[0] And, in fact, the RPC server we're connecting to does not expect them nor send them, and it fails when encountering them. With jsonrpc.el being based on EIEIO and generic methods, we work around this easily enough by defining our own process connection type subclassing jsonrpc-process-connection with associated methods that omit the header-related code (not sending the header with requests, and not looking for one when parsing responses, just trying to parse the incoming text as JSON). This seems to work well, and the code is pleasantly simple. However, we wonder if jsonrpc.el is not quite as generic as it should be, i.e. whether the use of these HTTP-style headers should be done in a specific subclass rather than the jsonrpc-process-connection one, so that other, non-LSP JSON-RPC servers could be used more easily. Would that be reasonable, or am I missing something (which wouldn't be surprising, as some of this is new territory for me)? I hope I'm not retreading old ground by bringing this up; I searched the list and bug tracker but couldn't find anything relevant. Thanks, Adam 0: https://www.jsonrpc.org/specification ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP 2023-10-18 23:17 jsonrpc.el uses "Content-Length" headers which seem specific to LSP Adam Porter @ 2023-10-19 0:11 ` João Távora 2023-10-19 20:59 ` Adam Porter 0 siblings, 1 reply; 6+ messages in thread From: João Távora @ 2023-10-19 0:11 UTC (permalink / raw) To: Adam Porter; +Cc: emacs-devel On Thu, Oct 19, 2023 at 12:17 AM Adam Porter <adam@alphapapa.net> wrote: > However, we wonder if jsonrpc.el is not quite as generic as it should > be, i.e. whether the use of these HTTP-style Instead of jsonrpc.el, I think you mean jsonrpc-process-connection. It's a subclass of jsonrpc-connection. If you want more genericity, you could subclass jsonrpc-connection. To give you an example, in a private project I subclassed jsonrpc-connection to make a websocket-based JSONRPC transport. It was really easy (I can show the code if you're interested). Presumably you're interested in using an Emacs pipe or network process as the transport, so by doing that you'd find yourself reinventing lots of stuff in the methods for jsonrpc-process-connection. Let's label this idea A. Now, the flaw I will concede about jsonrpc-process-connection is that it describes itself as a "jsonrpc connection over an Emacs process", but it would be more accurately described as "JSONRPC connection over HTTP-like over Emacs process". Presumably, you want the first and the last part but not the middle. I guess we could express the existing jsonrpc-process-connection as a mixin of HTTP-like behaviour and Emacs process behaviour. If we did that, you could more elegantly do your own hyperdrive-jsonrpc-connection by mixing in just the classes you wanted. So this is idea B. Back to idea A, we could just bundle most of the functionality used by the generics operating on jsonrpc-process-connection as free helper functions. Then you could just easily build on top of jsonrpc-connection the process-related stuff you want to do and the existing jsonrpc-process-connection would also use these helpers. Nevertheless, I think idea B is cleaner. From reading your email, you seem to agree with me. But I have to see your code first. You seem to have already come up with some kind of idea C. You say the code is "pleasantly simple", yet you still think it could be easier, so I don't know. I haven't seen your pleasantly-simple-but-not-quite-optimal code. I do know subclassing and removing behaviour violates the "is a" of inheritance and sometimes brings you into trouble later on. Or maybe not. So, to summarize, changes to the transport implementor's API in of jsonrpc.el are possible, so long as the current API isn't broken and the new things, be it helpers of idea A or new classes of idea B are documented in the manual. So show me your code and propose some patch to jsonrpc.el showing how much it would simplify your existing code. João ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP 2023-10-19 0:11 ` João Távora @ 2023-10-19 20:59 ` Adam Porter 2023-10-20 0:02 ` João Távora 0 siblings, 1 reply; 6+ messages in thread From: Adam Porter @ 2023-10-19 20:59 UTC (permalink / raw) To: João Távora; +Cc: emacs-devel, Joseph Turner [-- Attachment #1: Type: text/plain, Size: 3511 bytes --] Hi João, On 10/18/23 19:11, João Távora wrote: > Instead of jsonrpc.el, I think you mean jsonrpc-process-connection. It's > a subclass of jsonrpc-connection. If you want more genericity, you could > subclass jsonrpc-connection. Yes, thanks. > To give you an example, in a private project I subclassed > jsonrpc-connection to make a websocket-based JSONRPC transport. > It was really easy (I can show the code if you're interested). Sure, that could be helpful. > Presumably you're interested in using an Emacs pipe or network process > as the transport, so by doing that you'd find yourself reinventing lots of > stuff in the methods for jsonrpc-process-connection. Let's label this idea A. Yes, we're using a network process connecting to a local TCP server. Indeed, we didn't want to reinvent those things. > Now, the flaw I will concede about jsonrpc-process-connection is that it > describes itself as a "jsonrpc connection over an Emacs process", but it > would be more accurately described as "JSONRPC connection over HTTP-like > over Emacs process". Presumably, you want the first and the last part > but not the middle. Right, the code we wrote (shared below) basically removes those middle parts. > I guess we could express the existing > jsonrpc-process-connection as a mixin of HTTP-like behaviour and Emacs > process behaviour. If we did that, you could more elegantly do your > own hyperdrive-jsonrpc-connection by mixing in just the classes you wanted. > So this is idea B. > > Back to idea A, we could just bundle most of the functionality used by > the generics operating on jsonrpc-process-connection as free helper > functions. Then you could just easily build on top of jsonrpc-connection > the process-related stuff you want to do and the existing > jsonrpc-process-connection would also use these helpers. > > Nevertheless, I think idea B is cleaner. From reading your email, > you seem to agree with me. Right, we wanted to avoid the refactoring that would require. > But I have to see your code first. You seem to have already come up > with some kind of idea C. You say the code is "pleasantly simple", > yet you still think it could be easier, so I don't know. I haven't > seen your pleasantly-simple-but-not-quite-optimal code. Of course. Attaching that code below. :) > I do know subclassing and removing behaviour violates the "is a" > of inheritance and sometimes brings you into trouble > later on. Or maybe not. Yes, that is an ugly compromise. For the minor changes we had to make, it seemed reasonable within our project, but we'd be glad if it weren't necessary. > So, to summarize, changes to the transport implementor's API in > of jsonrpc.el are possible, so long as the current API isn't > broken and the new things, be it helpers of idea A or new classes of > idea B are documented in the manual. So show me your code and propose > some patch to jsonrpc.el showing how much it would simplify your > existing code. So, here's our code. As you can see, it just copies some forms from jsonrpc and removes the parts related to the HTTP-like headers. It seems to work so far, but I don't know if it's good or close to correct. :) Of course, one of the considerations is backward compatibility with existing users of jsonrpc. There don't appear to be very many yet, other than Eglot, so some kind of transition might be feasible, especially since jsonrpc.el is on GNU ELPA. Thanks for your work and your help with this. Adam [-- Attachment #2: jsonrpc-hacks.el --] [-- Type: text/x-emacs-lisp, Size: 1862 bytes --] ;; This code basically copies directly from `jsonrpc' while removing ;; some parts related to sending the HTTP-like headers. (defclass hyperdrive-jsonrpc-process-connection (jsonrpc-process-connection) nil) (cl-defmethod initialize-instance ((conn hyperdrive-jsonrpc-process-connection) slots) (cl-call-next-method) (cl-destructuring-bind (&key ((:process proc)) name &allow-other-keys) slots (set-process-filter proc #'hyperdrive-jsonrpc--process-filter))) (defun hyperdrive-jsonrpc--process-filter (proc string) (when (buffer-live-p (process-buffer proc)) (with-current-buffer (process-buffer proc) (let* ((inhibit-read-only t) (connection (process-get proc 'jsonrpc-connection))) ;; Insert the text, advancing the process marker. (save-excursion (goto-char (process-mark proc)) (insert string) (set-marker (process-mark proc) (point))) (condition-case err (when-let ((json-message (jsonrpc--json-read))) (with-temp-buffer (jsonrpc-connection-receive connection json-message))) (error (message "hyperdrive-jsonrpc--process-filter ERROR:%S PROC:%S STRING:%S" err proc string))))))) (cl-defmethod jsonrpc-connection-send ((connection hyperdrive-jsonrpc-process-connection) &rest args &key _id method _params _result _error _partial) "Send MESSAGE, a JSON object, to CONNECTION." (when method (plist-put args :method (cond ((keywordp method) (substring (symbol-name method) 1)) ((and method (symbolp method)) (symbol-name method))))) (let* ((message `(:jsonrpc "2.0" ,@args)) (json (jsonrpc--json-encode message))) (process-send-string (jsonrpc--process connection) json) (jsonrpc--log-event connection message 'client))) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP 2023-10-19 20:59 ` Adam Porter @ 2023-10-20 0:02 ` João Távora 2023-11-22 22:40 ` Adam Porter 0 siblings, 1 reply; 6+ messages in thread From: João Távora @ 2023-10-20 0:02 UTC (permalink / raw) To: Adam Porter; +Cc: emacs-devel, Joseph Turner Adam Porter <adam@alphapapa.net> writes: >> To give you an example, in a private project I subclassed >> jsonrpc-connection to make a websocket-based JSONRPC transport. >> It was really easy (I can show the code if you're interested). > > Sure, that could be helpful. Shown at the bottom of this message after my sig. I'm not showing the websocket library or the full thing, of course, but it should give you an idea. > Yes, we're using a network process connecting to a local TCP > server. Indeed, we didn't want to reinvent those things. > >> Now, the flaw I will concede about jsonrpc-process-connection is that it >> describes itself as a "jsonrpc connection over an Emacs process", but it >> would be more accurately described as "JSONRPC connection over HTTP-like >> over Emacs process". Presumably, you want the first and the last part >> but not the middle. > > Right, the code we wrote (shared below) basically removes those middle > parts. After having looked at your code, I think you may be underestimating the use of the Content-Length header. It's not a spurious decoration. Rather it helps processes make sense of incomplete or multiple messages that may appear in the process buffer. I'd say it's only in toy examples or "lucky" cases in which you can safely jsonrpc--json-read one single full JSON objects to completeness. In other words, with or without refactorings or adjustments to jsonrpc.el's design, I don't think this code can work reliably within Emacs: (defun hyperdrive-jsonrpc--process-filter (proc string) (when (buffer-live-p (process-buffer proc)) (with-current-buffer (process-buffer proc) (let* ((inhibit-read-only t) (connection (process-get proc 'jsonrpc-connection))) ;; Insert the text, advancing the process marker. (save-excursion (goto-char (process-mark proc)) (insert string) (set-marker (process-mark proc) (point))) (condition-case err (when-let ((json-message (jsonrpc--json-read))) (with-temp-buffer (jsonrpc-connection-receive connection json-message))) (error (message "hyperdrive-jsonrpc--process-filter ERROR:%S PROC:%S STRING:%S" err proc string))))))) For example, what if two full messages appear in the process buffer suddenly and the other endpoint is waiting for a reply to the second one before it sends more stuff? Isn't that deadlock? What if a very large message appears in chunks? Are you going to waste lots of works trying to read it and failing each time? I'd be careful about these things, but if you can absolutely guarantee that this never happens or somehow change the code to deal with them reliably, then OK. In any case, I'm now starting to think your code doesn't motivate any significant reworking of the class hierarchy in jsonrpc.el. It's true it uses some jsonrpc.el internals when it shouldn't but it's quite OK to augment the jsonrpc.el API to export jsonrpc--json-read and the few other internal helpers you're using. That doesn't pose any backward-compatibility challenge and your code is indeed quite simple. > Of course, one of the considerations is backward compatibility with > existing users of jsonrpc. There don't appear to be very many yet, > other than Eglot, so some kind of transition might be feasible, > especially since jsonrpc.el is on GNU ELPA. There are definitely other users, some not open source, but some are open source. Last time I did a GitHub code search I came up with at least couple non-Eglot uses. Not sure what you mean by "transition", but any refactorings/redesigns must not introduce breaking changes. João (defclass nih--connection (jsonrpc-connection) ((repl :accessor nih--repl :documentation "REPL buffer") (target-info :reader nih--target-info :initarg :target-info :documentation "Full target info. A JSON object returned by /json/list. May be augmented with nih-specific fields.") (socket :reader nih--socket :initarg :socket :documentation "Open WEBSOCKET object")) :documentation "Represents a NIH connection. `jsonrpc-name' is NOT unique.") (cl-defmethod jsonrpc-connection-send ((conn nih--connection) &rest args &key _id method _params _result _error _partial) "Send MESSAGE, a JSON object, to CONNECTION." ;; next form clearly something to put in an :around method (when method (plist-put args :method (cond ((keywordp method) (substring (symbol-name method) 1)) ((and method (symbolp method)) (symbol-name method))))) (let* ((message `(;; CDP isn't technically JSONRPC, so don't send ;; the `:jsonrpc' "2.0" version identifier which ;; trips up node's server, for example. ,@args)) (json (jsonrpc--json-encode message))) (with-slots (socket) conn (websocket-send-text socket json)) ;; also something for a generic :AFTER (jsonrpc--log-event conn message 'client))) (cl-defmethod jsonrpc-running-p ((conn nih--connection)) (with-slots (socket) conn (websocket-openp socket))) (cl-defmethod jsonrpc-shutdown ((conn nih--connection)) (with-slots (socket) conn (websocket-close socket))) (defun nih--on-websocket-message (ws frame) "Called when FRAME received on NIH WS." (jsonrpc-connection-receive (websocket-client-data ws) ; the connection (json-parse-string (websocket-frame-text frame) :object-type 'plist :null-object nil :false-object :json-false))) ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP 2023-10-20 0:02 ` João Távora @ 2023-11-22 22:40 ` Adam Porter 2023-12-01 0:45 ` João Távora 0 siblings, 1 reply; 6+ messages in thread From: Adam Porter @ 2023-11-22 22:40 UTC (permalink / raw) To: João Távora; +Cc: emacs-devel, Joseph Turner Hi João, Thanks for this reply of yours from last month. I hadn't had anything to add until now, after we've had more development in our project. On 10/19/23 19:02, João Távora wrote: > After having looked at your code, I think you may be underestimating > the use of the Content-Length header. It's not a spurious > decoration. Rather it helps processes make sense of incomplete or > multiple messages that may appear in the process buffer. I'd say > it's only in toy examples or "lucky" cases in which you can safely > jsonrpc--json-read one single full JSON objects to completeness. Agreed. Our intention was to use the hack of trying to parse potentially incomplete objects for only small responses, and only because our RPC server doesn't support the Content-Length header. We may be able to add support for that header to the RPC server, but we've been discussing another potential solution: to use something like the JSON Lines[0] (aka NDJSON[1]) format, which would mean that each JSON object would be sent as a single line (with any internal newlines being escaped). Then, in the process filter, we would look for unescaped newlines, and if we find any, we would parse the received data up to that newline. This would allow us to know when a complete object has been received. > It's true it uses some jsonrpc.el internals when it shouldn't but > it's quite OK to augment the jsonrpc.el API to export > jsonrpc--json-read and the few other internal helpers you're using. > That doesn't pose any backward-compatibility challenge and your code > is indeed quite simple. Of course, this means that we would still need to make our own version of the jsonrpc process filter function, similar to the code seen earlier in this thread, but it should require only minor changes. Given your experience with these things, does this seem like a reasonable solution to you? Or is there still strong reason to prefer using a Content-Length header to detect the end of responses? Thanks for your help! --Adam 0: https://jsonlines.org/ 1: https://ndjson.org/ ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP 2023-11-22 22:40 ` Adam Porter @ 2023-12-01 0:45 ` João Távora 0 siblings, 0 replies; 6+ messages in thread From: João Távora @ 2023-12-01 0:45 UTC (permalink / raw) To: Adam Porter; +Cc: emacs-devel, Joseph Turner Hi Adam, On Wed, Nov 22, 2023 at 10:41 PM Adam Porter <adam@alphapapa.net> wrote: > Thanks for this reply of yours from last month. I hadn't had anything > to add until now, after we've had more development in our project. Sorry for the delay from my side. > Given your experience with these things, does this seem like a > reasonable solution to you? I can't see why it wouldn't work, which just means I can't immediately find a flaw. I know from experience there are subtleties very easy to miss in these process filters. > Or is there still strong reason to prefer > using a Content-Length header to detect the end of responses? The drawbacks I see is (1) having to sweep through the data of every received chunk to look for the newline message separator and (2) all the escaping and un-escaping work. Whereas with Content-Length you read a number once, and continuously subtract from it to know if you have a full message. If you do, that's when json-parse-buffer is called, and that's when you sweep through the data to construct the JSON object. It's a more complicated parser but it's already written and it's proven sturdy. If I were coding this in C/C++ with performance and memory concerns it's also probably how I would go about it Here, I don't know if it makes a difference, you'd have to measure. Personally, if the other endpoint was under my control, I'd just add Content-Length for peace of mind. Seems easier than adding escaping anyways. Not sure if this reason is strong enough. João ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-01 0:45 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-18 23:17 jsonrpc.el uses "Content-Length" headers which seem specific to LSP Adam Porter 2023-10-19 0:11 ` João Távora 2023-10-19 20:59 ` Adam Porter 2023-10-20 0:02 ` João Távora 2023-11-22 22:40 ` Adam Porter 2023-12-01 0:45 ` João Távora
Code repositories for project(s) associated with this public inbox https://git.savannah.gnu.org/cgit/emacs.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).