jsonrpc.el uses "Content-Length" headers which seem specific to LSP

jsonrpc.el uses "Content-Length" headers which seem specific to LSP

[Adam Porter]

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

Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP

[João Távora]

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

Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP

[Adam Porter]

[-- 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)))

Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP

[João Távora]

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)))

Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP

[Adam Porter]

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/

Re: jsonrpc.el uses "Content-Length" headers which seem specific to LSP

[João Távora]

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