Open tshemsedinov opened 7 years ago
Thanks for putting this up!
I have some comments to share.
Where's the "More" flag? We won't be able to send messages that don't fit in one chunk without it.
I thought we have come to the conclusion that many of these headers are only meaningful for the first chunk of a stream or a message and transmitting them all the time is superfluous, so there should be a field indicating the chunk type (with at least stream/message start and continuation being ones of them), and the headers would differ for various types, haven't we? Please correct me if I'm wrong.
Length Description Value ...,
Headers (comma-terminated)
Doing so is certainly a possibility, yes, but what we have discussed and agreed upon was that this information is encoded and decoded using the format specified in the "Chunk encoding" field, i.e., JSTP uses comma-separated values, JSON is a mere representation of the full message object without the type field which is now encoded in the binary headers (e.g., { "id": 42, "interface": "magic", "method": "spawnKitten", "payload": ["white"] }
or { "callId": 42, "success": true, "payload": [{ "kittenHandle": 18741234 }] }
), some other format would possibly encode all of these fields in a binary form.
What the proposal in this issue specifies, however, is that only the payload serialization format is customizable. While I have some thoughts about it which I'd like to discuss in person, one thing I'd like to point out right away is that if we go this route, I'd vouch for adding a new 4-octet field to the chunk headers. It will be used as an id for call
s, inspect
s and their corresponding callback
s, and ping
s and pong
s will store total counters of received messages to be shared with the other party (that's some voodoo we need for sessions, yeah). For the rest of message types it will remain reserved, yet with certain possibility to be used in the future (as an example, event
s might have associated TTL values at some point). Corresponding fields in their headers will then go away from the chunk body, so we won't have to waste CPU cycles on parsing numbers while we already have a binary structure a few bytes away in memory.
By the way:
Length Description Value ...,
Headers (comma-terminated)
I'd like to avoid the term "headers" used for these fields, since we already use it for binary headers located in the beginning of a chunk. Or, idk, never use it without an adjective: "chunk headers" or "message headers"/"stream headers".
Message type Description Value S binary stream start 83 s binary stream more 115
As I mentioned previously, either "start" and "more" are a thing for any chunk regardless of its type, or no RPC messages with long payload. I find it particularly strange to see this decision reverted, because I was initially thinking of something like the proposal currently states on this matter and even explicitly said things like "1 chunk = 1 message", but, IIRC, it was you who proved me wrong and demonstrated a clear evidence that messages should be continuable too :)
Also, I really liked your idea to make messages just streams (of course, given that by default streams are lightweight and do not lead to instantiating heavy-weight machinery like Node.js streams unless the chunk type says it is a binary stream of data), that would simplify things a lot.
callId
field is missing. Also, copy-paste mistake: eventName
-> methodName
.
Corresponding callId
field is missing.
Also, should we add a boolean field indicating success and make payload either a result or an error depending on it, or turn payload into [error, ...results]
? [ref]
I'd prefer a boolean field, but this is certainly something we should discuss.
inspectId
field is missing. No payload exists for this message type.
inspectId
field is missing. success
field should maybe be added (see the section about callbacks).
pingId
and receivedCount
fields are missing.
Will discuss later in person.
@aqrln, now that you mentioned the fact that we forgot about the callback status, I thought that it could be nice to have one octet in the callback header to encode something like 'return code' (which is similar to the 'exit code' in Unix), where some of the values are reserved by the protocol, such as 0 indicating success and other default error codes we have now, while others are available to be used by the user of the protocol. This way, users can also provide optional payload with their error codes.
Here is a combination of new JSTP packet format and yesterdays cccp binary proposals. @metarhia/jstp-core
Chunk
H,h,E,C,c,I,i,P,p,S,s
...,
Message type
Handshake request
...,
...,
...,
Handshake response
Event
...,
...,
Call
...,
...,
Callback
Inspect request
...,
Inspect response
Ping and Pong
Binary stream start
...,
Binary stream more