Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split input stream according to JSON structure, not by newlines #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

onnokort
Copy link
Contributor

For example, jsonrpc4j will not output newlines for each JSON structure it
sends.

@deavid
Copy link
Owner

deavid commented Apr 28, 2015

I understand you're trying to compatibilize bjsonrpc with jsonrpc4j, which is good.
But bjsonrpc was aimed for speed, so '\n' split was a decision to avoid understanding json in the first layer, which should give better performance than your solution.
Also, other jsonrpc libraries are HTTP, and not socket based, so they are relying in HTTP protocol to separate different calls.
And finally, and most important here: you will break client-server compatibility between versions with that change. And that's unacceptable.

I suggest to create this as an option when starting bjsonrpc (as an option to connect, or a library-wide option). And that option should default to "split by lines". This way you (and others) can use this library to interface jsonrpc4j adding a simple line at the start of the program, and the rest of the users won't notice anything.

If you know if the JSONRPC standard says anything about how to split the messages, please tell me.

Thanks for your patch. It looks really good

…lines

Connection objects have a new field 'split' which is a function that will be
used to split up the incoming JSON objects. The default behavior is to split
by newline, but it can be switched to separate the stream by disassembling
into JSON chunks by setting this field to 'Connection.split_by_json'.

This makes bjsonrpc compatible with implementations that do not use newlines
to separate JSON objects, such as jsonrpc4j.
@onnokort
Copy link
Contributor Author

Hey,

I understand you're trying to compatibilize bjsonrpc with jsonrpc4j, which is good.
But bjsonrpc was aimed for speed, so '\n' split was a decision to avoid understanding json in the first layer, which should give better performance than your solution.

Understood. Yes, performance is better with the original code, for execution of 100x your test suite, I have:

  • 38.0s with the original code
  • 44.2s with the JSON-aware splitter
  • 38.0s with the updated PR request that allows configuration (see below)

Also, other jsonrpc libraries are HTTP, and not socket based, so they are relying in HTTP protocol to separate different calls.
And finally, and most important here: you will break client-server compatibility between versions with that change. And that's unacceptable.

Hmm, I was assuming that there is sane JSON structure expected on the wire anyways, so it should not do any harm as long as the JSON splitter works as well. Or are you saying that it will break in the HTTP-specific code path? In that case, I'd suggest that I add a corresponding warning 'JSON splitter won't work with HTTP' to the appropriate docs.

I suggest to create this as an option when starting bjsonrpc (as an option to connect, or a library-wide option). And that option should default to "split by lines".
This way you (and others) can use this library to interface jsonrpc4j adding a simple line at the start of the program, and the rest of the users won't notice anything.

Yes, sounds good, I have just done that and updated the PR accordingly. I am not sure whether style this is in the spirit of your library, though. I also added an option to initialize all Connection objects thusly in the server code. These changes does not seem to impact speed of the newline-splitter at all, at least from my limited tests above.
I ran your test suite successfully in all configurations and I tested a configuration of some of my own Java code talking to a bjsonrpc-server. I didn't test HTTP transport specifically - is there anything I should do?

If you know if the JSONRPC standard says anything about how to split the messages, please tell me.

I was reading into this a bit. I found similar thoughts to my own at http://www.simple-is-better.org/json-rpc/transport_sockets.html, where he goes into splitting by netstrings even. I did not find any reference to THE way how to split. The standard seems to be incomplete here, I guess?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants