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.

Sign up for GitHub

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

Improve RPC message encoding #11294

Merged
merged 2 commits into from
Jun 21, 2022
Merged

Improve RPC message encoding #11294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbf0060
Improve RPC message encoding
tortmayr Jun 14, 2022
8e9f4de
Address review feedback
tortmayr Jun 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/core/src/browser/logger-frontend-module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { ILogger, Logger, LoggerFactory, setRootLogger, LoggerName, rootLoggerNa
import { LoggerWatcher } from '../common/logger-watcher';
import { WebSocketConnectionProvider } from './messaging';
import { FrontendApplicationContribution } from './frontend-application';
import { EncodingError } from '../common/message-rpc/rpc-message-encoder';

export const loggerFrontendModule = new ContainerModule(bind => {
bind(FrontendApplicationContribution).toDynamicValue(ctx => ({
Expand All @@ -39,7 +40,13 @@ export const loggerFrontendModule = new ContainerModule(bind => {
if (property === 'log') {
return (name, logLevel, message, params) => {
ConsoleLogger.log(name, logLevel, message, params);
return target.log(name, logLevel, message, params);
return target.log(name, logLevel, message, params).catch(err => {
if (err instanceof EncodingError) {
// In case of an EncodingError no RPC call is sent to the backend `ILoggerServer`. Nevertheless, we want to continue normally.
return;
}
throw err;
});
};
}
return target[property];
Expand Down
17 changes: 15 additions & 2 deletions packages/core/src/common/message-rpc/rpc-message-encoder.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import { expect } from 'chai';
import { Uint8ArrayReadBuffer, Uint8ArrayWriteBuffer } from './uint8-array-message-buffer';
import { RpcMessageDecoder, RpcMessageEncoder } from './rpc-message-encoder';
import { EncodingError, RpcMessageDecoder, RpcMessageEncoder } from './rpc-message-encoder';

describe('PPC Message Codex', () => {
describe('RPC Message Encoder & Decoder', () => {
Expand All @@ -26,8 +26,11 @@ describe('PPC Message Codex', () => {

const encoder = new RpcMessageEncoder();
const jsonMangled = JSON.parse(JSON.stringify(encoder));
// The RpcMessageEncoder can decode/encode collections, whereas JSON.parse can't. => We have to manually restore the set
// eslint-disable-next-line @typescript-eslint/no-explicit-any
jsonMangled.registeredTags = (encoder as any).registeredTags;

encoder.writeTypedValue(writer, encoder);
encoder.writeTypedValue(writer, encoder, new WeakSet());

const written = writer.getCurrentContents();

Expand All @@ -38,5 +41,15 @@ describe('PPC Message Codex', () => {

expect(decoded).deep.equal(jsonMangled);
});
it('should fail with an EncodingError when trying to encode the object ', () => {
const x = new Set();
const y = new Set();
x.add(y);
y.add(x);
const encoder = new RpcMessageEncoder();

const writer = new Uint8ArrayWriteBuffer();
expect(() => encoder.writeTypedValue(writer, x, new WeakSet())).to.throw(EncodingError);
});
});
});
109 changes: 82 additions & 27 deletions packages/core/src/common/message-rpc/rpc-message-encoder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,18 @@ export enum ObjectType {
Number = 7,
// eslint-disable-next-line @typescript-eslint/no-shadow
ResponseError = 8,
Error = 9
Error = 9,
Map = 10,
Set = 11,
Function = 12

}

/**
* A value encoder writes javascript values to a write buffer. Encoders will be asked
* in turn (ordered by their tag value, descending) whether they can encode a given value
* This means encoders with higher tag values have priority. Since the default encoders
* have tag values from 1-7, they can be easily overridden.
* have tag values from 1-12, they can be easily overridden.
*/
export interface ValueEncoder {
/**
Expand All @@ -119,11 +122,12 @@ export interface ValueEncoder {
* Write the given value to the buffer. Will only be called if {@link is(value)} returns true.
* @param buf The buffer to write to
* @param value The value to be written
* @param visitedObjects The collection of already visited (i.e. encoded) objects. Used to detect circular references
* @param recursiveEncode A function that will use the encoders registered on the {@link MessageEncoder}
* to write a value to the underlying buffer. This is used mostly to write structures like an array
* without having to know how to encode the values in the array
*/
write(buf: WriteBuffer, value: any, recursiveEncode?: (buf: WriteBuffer, value: any) => void): void;
write(buf: WriteBuffer, value: any, visitedObjects: WeakSet<object>, recursiveEncode?: (buf: WriteBuffer, value: any, visitedObjects: WeakSet<object>) => void): void;
tsmaeder marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand All @@ -141,6 +145,16 @@ export interface ValueDecoder {
read(buf: ReadBuffer, recursiveDecode: (buf: ReadBuffer) => unknown): unknown;
}

/**
* Custom error thrown by the {@link RpcMessageEncoder} if an error occurred during the encoding and the
* object could not be written to the given {@link WriteBuffer}
*/
export class EncodingError extends Error {
constructor(msg: string) {
super(msg);
}
}

/**
* A `RpcMessageDecoder` parses a a binary message received via {@link ReadBuffer} into a {@link RpcMessage}
*/
Expand All @@ -149,6 +163,10 @@ export class RpcMessageDecoder {
protected decoders: Map<number, ValueDecoder> = new Map();

constructor() {
this.registerDecoders();
}

registerDecoders(): void {
this.registerDecoder(ObjectType.JSON, {
read: buf => {
const json = buf.readString();
Expand Down Expand Up @@ -207,6 +225,18 @@ export class RpcMessageDecoder {
read: buf => buf.readNumber()
});

this.registerDecoder(ObjectType.Map, {
read: buf => new Map(this.readArray(buf))
});

this.registerDecoder(ObjectType.Set, {
read: buf => new Set(this.readArray(buf))
});

this.registerDecoder(ObjectType.Function, {
read: () => ({})
});

}

/**
Expand Down Expand Up @@ -257,9 +287,7 @@ export class RpcMessageDecoder {
protected parseRequest(msg: ReadBuffer): RequestMessage {
const callId = msg.readUint32();
const method = msg.readString();
let args = this.readArray(msg);
// convert `null` to `undefined`, since we don't use `null` in internal plugin APIs
args = args.map(arg => arg === null ? undefined : arg); // eslint-disable-line no-null/no-null
const args = this.readArray(msg);

return {
type: RpcMessageType.Request,
Expand All @@ -272,9 +300,7 @@ export class RpcMessageDecoder {
protected parseNotification(msg: ReadBuffer): NotificationMessage {
const callId = msg.readUint32();
const method = msg.readString();
let args = this.readArray(msg);
// convert `null` to `undefined`, since we don't use `null` in internal plugin APIs
args = args.map(arg => arg === null ? undefined : arg); // eslint-disable-line no-null/no-null
const args = this.readArray(msg);

return {
type: RpcMessageType.Notification,
Expand Down Expand Up @@ -348,9 +374,14 @@ export class RpcMessageEncoder {
}
});

this.registerEncoder(ObjectType.Function, {
is: value => typeof value === 'function',
write: () => { }
});

this.registerEncoder(ObjectType.Object, {
is: value => typeof value === 'object',
write: (buf, object, recursiveEncode) => {
write: (buf, object, visitedObjects, recursiveEncode) => {
const properties = Object.keys(object);
const relevant = [];
for (const property of properties) {
Expand All @@ -363,7 +394,7 @@ export class RpcMessageEncoder {
buf.writeLength(relevant.length);
for (const [property, value] of relevant) {
buf.writeString(property);
recursiveEncode?.(buf, value);
recursiveEncode?.(buf, value, visitedObjects);
}
}
});
Expand All @@ -388,6 +419,16 @@ export class RpcMessageEncoder {
write: (buf, value) => buf.writeString(JSON.stringify(value))
});

this.registerEncoder(ObjectType.Map, {
is: value => value instanceof Map,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we have to treat anything that can hold a reference the same as the typeof x === object? I mean I can do

const x= new Set();
const y= new Set();
x.add(y);
y.add(x);

Bingo, circular reference 🤷

Copy link
Contributor

@colin-grant-work colin-grant-work Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sets aren't a problem here, but JavaScript is, since if you want to, you can treat almost anything as an object:

const a = new Set();
// undefined
typeof a
// 'object'
const b = 'hello'
// undefined
const c = 'other string'
// undefined
 b.c = c;
// 'other string'
 c.b = b
// 'hello'
typeof b
// 'string'

TypeScript wouldn't allow exactly that syntax, but it would be happy with this:

const d = 'yet another string'
// undefined
const e = 'a fourth string'
// undefined
Object.assign(d, {e});
// [String: 'yet another string'] { e: 'a fourth string' }
typeof d
// 'string'

But as long as we have know where the border is between things whose fields we'll visit (all object types other than null?) and those whose fields we won't (primitives, functions), and we use the reference checker for the former, it should be safe - i.e. we need to make sure that all handlers of object types are delegates of the object handler.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I have refactored the code and now all object types (other than null/undefined) are going through the reference check. I have also added a test case for circular references based on the example that @tsmaeder provided.

write: (buf, value: Map<any, any>, visitedObjects) => this.writeArray(buf, Array.from(value.entries()), visitedObjects)
});

this.registerEncoder(ObjectType.Set, {
is: value => value instanceof Set,
write: (buf, value: Set<any>, visitedObjects) => this.writeArray(buf, [...value], visitedObjects)
});

this.registerEncoder(ObjectType.Undefined, {
// eslint-disable-next-line no-null/no-null
is: value => value == null,
Expand All @@ -396,8 +437,8 @@ export class RpcMessageEncoder {

this.registerEncoder(ObjectType.ObjectArray, {
is: value => Array.isArray(value),
write: (buf, value) => {
this.writeArray(buf, value);
write: (buf, value, visitedObjects) => {
this.writeArray(buf, value, visitedObjects);
}
});

Expand Down Expand Up @@ -454,44 +495,58 @@ export class RpcMessageEncoder {
buf.writeUint8(RpcMessageType.Notification);
buf.writeUint32(requestId);
buf.writeString(method);
this.writeArray(buf, args);
this.writeArray(buf, args, new WeakSet());
}

request(buf: WriteBuffer, requestId: number, method: string, args: any[]): void {
buf.writeUint8(RpcMessageType.Request);
buf.writeUint32(requestId);
buf.writeString(method);
this.writeArray(buf, args);
this.writeArray(buf, args, new WeakSet());
}

replyOK(buf: WriteBuffer, requestId: number, res: any): void {
buf.writeUint8(RpcMessageType.Reply);
buf.writeUint32(requestId);
this.writeTypedValue(buf, res);
this.writeTypedValue(buf, res, new WeakSet());
}

replyErr(buf: WriteBuffer, requestId: number, err: any): void {
buf.writeUint8(RpcMessageType.ReplyErr);
buf.writeUint32(requestId);
this.writeTypedValue(buf, err);
this.writeTypedValue(buf, err, new WeakSet());
}

writeTypedValue(buf: WriteBuffer, value: any): void {
for (let i: number = this.encoders.length - 1; i >= 0; i--) {
if (this.encoders[i][1].is(value)) {
buf.writeUint8(this.encoders[i][0]);
this.encoders[i][1].write(buf, value, (innerBuffer, innerValue) => {
this.writeTypedValue(innerBuffer, innerValue);
});
return;
writeTypedValue(buf: WriteBuffer, value: any, visitedObjects: WeakSet<object>): void {
if (value && typeof value === 'object') {
if (visitedObjects.has(value)) {
throw new EncodingError('Object to encode contains circular references!');
}
visitedObjects.add(value);
}

try {
for (let i: number = this.encoders.length - 1; i >= 0; i--) {
if (this.encoders[i][1].is(value)) {
buf.writeUint8(this.encoders[i][0]);
this.encoders[i][1].write(buf, value, visitedObjects, (innerBuffer, innerValue, _visitedObjects) => {
this.writeTypedValue(innerBuffer, innerValue, _visitedObjects);
});
return;
}
}
throw new EncodingError(`No suitable value encoder found for ${value}`);
} finally {
if (value && typeof value === 'object') {
visitedObjects.delete(value);
}
}
}

writeArray(buf: WriteBuffer, value: any[]): void {
writeArray(buf: WriteBuffer, value: any[], visitedObjects: WeakSet<object>): void {
buf.writeLength(value.length);
for (let i = 0; i < value.length; i++) {
this.writeTypedValue(buf, value[i]);
this.writeTypedValue(buf, value[i], visitedObjects);
}
}
}