Split flatbuffers messages into Requests and Responses

[ry]

deno/src/msg.fbs

Lines 1 to 60 in c1de50b

	union Any {
	Start,
	StartRes,
	CodeFetch,
	CodeFetchRes,
	CodeCache,
	SetTimeout,
	Exit,
	Environ,
	EnvironRes,
	Fetch,
	FetchRes,
	MakeTempDir,
	MakeTempDirRes,
	Mkdir,
	Chmod,
	Remove,
	ReadFile,
	ReadFileRes,
	ReadDir,
	ReadDirRes,
	WriteFile,
	CopyFile,
	Rename,
	Readlink,
	ReadlinkRes,
	ReplStart,
	ReplStartRes,
	ReplReadline,
	ReplReadlineRes,
	Resources,
	ResourcesRes,
	Symlink,
	Stat,
	StatRes,
	SetEnv,
	Truncate,
	Open,
	OpenRes,
	Read,
	ReadRes,
	Write,
	WriteRes,
	Close,
	Shutdown,
	Listen,
	ListenRes,
	Accept,
	Dial,
	NewConn,
	Chdir,
	Cwd,
	CwdRes,
	Metrics,
	MetricsRes,
	Run,
	RunRes,
	RunStatus,
	RunStatusRes
	}

Currently we have a union of all message types called Any. The union is included in the Base message as an element called inner.

However there are two distinct types of messages: Requests and Responses. It would be better for type-checking and organizationally if we had two unions.

As an additional note: it seems many of our "table" messages could instead be "struct" messages. It would be interesting to explore that.

[kpelelis]

I am interested into investigating this topic, if no one is working on it :)

[bartlomieju]

I wanted to take a shot at it - I tried some prototyping wanting to split it into:

union Request {
   Start,
   CodeFetch
   ...
}

union Response {
   // 1.
   StartRes,
   CodeFetchRes,
   ...
}

table Base {
  cmd_id: uint32;
  sync: bool = false;
  error_kind: ErrorKind = NoError;
  error: string;
  // 2.
  inner: Request | Response;
}

1. Unions in flatbuffers reference tables, so if we want to have Request.Start we can't have Response.Start since they would reference the same table, OTOH Response.StartRes is bad.
2. You can't have union of unions or any other way to tell flatbuffers use this union or this union.

Possible solutions:

• define union MessageType and Base.inner: MessageType
• rename request messages to have Req prefix (eg. ReqStart)
• rename response messages to have Res prefix (eg. ResStart)

Not a huge gain but requires a lot of renaming. If I got anything wrong, please corret me. @ry do you still want to pursue it?

[ry]

@bartlomieju If it creates a bunch of churn, it's not worth it. I would punt unless we can find a way to do this without having to touch a bunch of code in ops.rs.

We're still not quite to the point where we can make a final decision on flatbuffers - they might be too slow. However, we're currently bottlenecked elsewhere (not in serialization/deserialization) so we have to work that out first. All that's to say: it's not impossible to imagine removing msg.fbs entirely in the future and replacing with serde (for example) so let's wait until that's ruled out before doing these final invasive clean ups. I'll leave this issue open.

[bartlomieju]

Got it, thanks 👍

[bartlomieju]

Close in favor of #2121?

CC @ry