Use readline to separate JSON response objects

[shesek]

• JSON responses are newline-delimited by c-lighting and can't contain newlines, so this achieves the same in a simpler and more efficient manner. incorrect, see Use readline to separate JSON response objects #4 (comment)

• The previous method would break with something like { "key": "}" }

• The previous method would break if a single JSON response is split across
  multiple data events (possible with large responses).

Note: this PR currently builds on top of #3. Once you decide whether to accept #3 or not, I'll rebase this PR to make it merge cleanly. To see just the diff relevant for this PR, view the d543474 commit.

[shesek]

@cdecker @rustyrussell is c-lightning writing one response per line a behavior that can be relied upon?

[shesek]

(forced-push updated to adhere to the xo linter rules)

[shesek]

It looks like I might've been mistaken about c-lightning JSON serialization and that a single response object could contain newlines. I'll look into that and update.

[isghe]

#4 (comment)

Yes, that's why we had to do that so much strange thing
Thanks a lot :-)

[shesek]

I'll try to figure out a safer way to implement this, or perhaps try and get c-lightning modified to write one message per line. (any chance to make that happen? @cdecker @rustyrussell)

[cdecker]

Yes, we may have newlines in the JSON reply, and we don't do length prefixes because we may end up streaming some results (meaning we can't know upfront how large the reply will be. For the golang client and the python client we resorted to opening a new connection for each request, we had to do this anyway to get any form of concurrent request support, and it is very cheap to open a connection to a local unix socket.

[shesek]

Yes, we may have newlines in the JSON reply

Is that by design? Can this be modified?

we resorted to opening a new connection for each request ... it is very cheap to open a connection to a local unix socket.

Will the c-lightning JSONRPC API not eventually be available over TCP as well?

we had to do this anyway to get any form of concurrent request support

Is that a limitation on c-lightning's side, or in the client?

[shesek]

If c-lighting isn't changed to write one message per line, message splitting should probably be implemented using a fully JSON-compliant streaming parser like jsonsp.

[cdecker]

Yes, we may have newlines in the JSON reply
Is that by design? Can this be modified?

We currently don't have any fields that contain newlines, but they might eventually be needed.

Will the c-lightning JSONRPC API not eventually be available over TCP as well?

No, it is unix sockets only for the foreseeable future, if we were to expose it over TCP we could add message framing (HTTP, ...), but currently we use sidecars that proxy requests to the daemon. It also allows us to do fine-grained access control on that level.

Is that a limitation on c-lightning's side, or in the client?

Thinking about this a bit more, it's actually not true, we can support multiple requests but we may have to have a mutex to make sure multiple requests/replies don't mangle each other. I wouldn't take it for granted at the moment that it works.

[shesek]

We currently don't have any fields that contain newlines, but they might eventually be needed.

Newlines inside JSON-formatted strings are encoded as \n, when would it be required to have an actual line-feed within a single message? Standard JSON encoding should always result in a single line (JavaScript's JSON.stringify() behaves like that, unless explicitly told to add indentation).

we can support multiple requests but we may have to have a mutex to make sure multiple requests/replies don't mangle each other.

I'm assuming that's on c-lightning side? On the client's side it should be possible to unmangle them using the request/response id property.

[afilini]

I personally don't really like the idea of splitting the data received using newlines; I agree with you that there is a clear margin to improve my initial (probably broken in some cases) implementation, but I would still prefer to use an algorithm to "look for" different JSON objects.

I also think that the idea of opening a new connection for every request is not really the "cleanest" solution, but if @cdecker can't guarantee that c-lightning supports multiple concurrent requests maybe this is the only way to do it, at least right now. What do you think about this?

[rustyrussell]

Nadav Ivgi <[email protected]> writes:

> We currently don't have any fields that contain newlines, but they might eventually be needed. Newlines inside JSON-formatted strings are encoded as `\n`, when would it be required to have an actual line-feed within a single message? Standard JSON encoding should always result in a single line (JavaScript's `JSON.stringify()` behaves like that, unless explicitly told to add indentation).

You're right, looking at json.org. I'll have to add escaping in case any ever leak through... Cheers, Rusty.