feat(node-runtime-worker-thread): Advanced serialization for evaluation result COMPASS-4557 #572
Conversation
…own inside an rpc helper
… by worker runtime
…back to the client
…e shell result values before returning them
| removeListener: messageBus.off.bind(messageBus), | ||
| postMessage(data) { | ||
| if (isClientMessageData(data) && Array.isArray(data.args)) { | ||
| // We don't guard against serialization errors on the client, if they |
There was a problem hiding this comment.
Who is the client here? the part of the node-runtime-worker-thread that runs outside of the thread or the end consumer?
There was a problem hiding this comment.
As this is a general RPC helper, I was writing comments using the server-client terminology: so like the client is whoever calls a method, server is whoever executes it and returns the result. What would you suggest instead? Local-remote I think is sometimes used for RPC, sounds better? 🤔
There was a problem hiding this comment.
Don't know, i think client server, if is used consistently should be ok. I would understand better if we call worker, or child process the part that executes the code probably, as is more in context, but by all means if we have client/server everywhere is fine.
What i missed here though is which part of the client is 'client' here? From the comment i thought that the serialization error is something that the browser-shell would have had to handle in a special way, but it seems the 'client part' of the rpc already does it.
There was a problem hiding this comment.
So what I was trying to communicate is that whoever is issuing an RPC request, will be responsible for handing the errors, whatever they are. If exposed method throws (on the server), we propagate the error back and re-throw, if caller throws (on the client, e.g., during serialization of arguments), we don't do anything and just throw. Re-reading this particular comment I think it's more confusing than helping, so I'll remove it
There was a problem hiding this comment.
Updated in 8a77045. Hopefully it's better now
|
|
||
| if (isServerMessageData(data)) { | ||
| data.res = serialize(data.res); | ||
| // If serialization error happened on the server, we use our special |
There was a problem hiding this comment.
I think this is clear now, is the part of the RPC protocol that is happening in the child process
| chai.use(chaiAsPromised); | ||
|
|
||
| const chaiBson: Chai.ChaiPlugin = (chai) => { | ||
| chai.Assertion.addMethod('bson', function bson(expected) { |
There was a problem hiding this comment.
Wow i wish i knew extending chai was so simple long ago 🤣
| const value: SerializedShellApiResult = result.printable; | ||
| const printable = EJSON.deserialize(value.serializedValue); | ||
|
|
||
| // Primitives should not end up here ever, but we need to convince TS that |
There was a problem hiding this comment.
Again i think it would be clearer if we take care of this if case right in the beginning of the function, keep it out of our way and mind in the rest of the code.
There was a problem hiding this comment.
It's the EJSON.deserialize return type (type SerializableTypes = Document | Array<JSONPrimitive | Document> | JSONPrimitive) that asserts primitive types on the printable, so this check has to be here
| await this.initWorkerPromise; | ||
| return this.childProcessRuntime.evaluate(code); | ||
| return deserializeEvaluationResult( | ||
| await this.childProcessRuntime.evaluate(code) |
| ClientMessageData | ||
| } from 'postmsg-rpc'; | ||
| import { | ||
| serialize, |
There was a problem hiding this comment.
are serialize and serializeError 2 separate functions cause we can't safely tell an error from another object?
There was a problem hiding this comment.
This, and also a bunch of properties on error are coming from prototype and non-enumerable, so they are just lost if we pass errors directly to v8.serialize without extracting them first
…should return only serializable parts of shell result on evaluation
…lper as the sole user of those methods
…d): Do not rely on non-enumerable, non-serializable properties in Cursor printable
gribnoysup
force-pushed
the
compass-4557-advanced-serialization
branch
from
2505e72 to
38be23a
Compare
|
|
||
| interface CursorIterationResultOutputProps { | ||
| value: Document[] & { cursorHasMore: boolean }; | ||
| value: { documents: Document[]; cursorHasMore: boolean }; |
gribnoysup
force-pushed
the
compass-4557-advanced-serialization
branch
from
4bc1ae8 to
4846f1e
Compare
…in rpc helper internally
…ace them with (hopefully) less confusing ones
|
|
||
| const ERROR = '$$ERROR'; | ||
|
|
||
| const MESSAGE = '$$MESSAGE'; |
There was a problem hiding this comment.
Super-hyper minor .. what about we qualify the name here TYPE_MESSAGE and TYPE_ERROR or similar?
3 participants
Adds better serialization support for the return value of runtime evaluation result.
v8.serialize/deserializeis a powerful module, but some things can't just be serialized when passed through the message channel between two threads (e.g., functions, class instances, custom errors, etc.). For this reason we are adding custom serialization that tries to save as much of important result data as possible. For everything that can be identified asshell-apiresult value, we serialize it as EJSON and collect all non-enumerable values, for everything else that is not a primitive, weinspectit right away and return an inspection result, as the amount of possible weird type combinations to deal with is too much and from thebrowser-replperspective it's not really important what was the value beforeinspect-ed