FluidFramework
FluidFramework copied to clipboard
Op processing: shift responsibilities across Loader & Runtime layers, fix bugs, reduce concepts
Note: This change represents direction of changes, and final (or rather one of the temp states on the way to cleanup all of these issues). It will not be merged as is - I'll split it across main & next changes to speed up process (it can be checked in as is into next, but it's too slow, and I need to deprecate some stuff in main to allow faster move).
This change primarily is about moving responsibility of packing / unpacking op contents from Loader to runtime. Before this change, stringification and parsing of contents was done by Loader. Given that contents (with one exception - Summarize op) is never examined by Loader layer, and in future we will move toward binary payloads, it makes sense for Loader not to do these conversions. As part of making these changes, I've found some bugs, unused code and definitions, as well as unwanted leakage of concepts. Some things are hard to change also because we quite often do not expose concepts at the right level and allow cross-layer connections that should be avoided, so some of the changes are forward looking. All the changes are done in a way to fully support back-compat (with exception of removed functions). This results in a lot of follow ups required, tracked via AB#1385 (appropriate comments are in the code, as well as more items tracked in this issue)
Key changes:
- ContainerRuntime is stringifying and parsing contents of ops.
- Loader will continue to do the same (conditionally) for back-compat
- Added IContainerContext.submitSummaryFn to more clearly define summarize contract.
- Noops are broken. We submit contents: "null" to describe non-immediate noop, while service excepts contents === null (not a string)!.
- Fixing it by splitting these ops to "noop" and "accept" ops. In future, service should treat all noops as non-immediate, and it may choce so (but not required) to do some level of coalescing for "accept" ops.
- Removed RuntimeMessage, isUnpackedRuntimeMessage. Telemetry added in prior changes shows that we do not hit these paths (old way of encoding runtime messages). Plus, change to move to new format was done in summary of 2020, before we went public.
- There was also duplication (ContainerMessageType), and wrong level of details exposed from runtime.
- Removed isSystemMessage() as not used
- Moved isClientMessage() to Loader layer (not exported). Only DeltaManager needs to know about that.
- ContainerRuntime's "op" event will expose all the ops, including non-runtime ops. This is done for future changes to remove DeltaManager's "op" event and reliance on it (as well as any other events like that).
- Removed MessageType.RoundTrip, MessageType.RemoteHelp as not used
- Documented ISequencedDocumentMessage.data and changed code parsing summary ack/nack ops to use data, not contents. relay service will stop writing contents for service ops.
- removed last argument (context) for IRuntime.process() - it was not used.
Bugs discovered and logged for follow up:
- AB#1386: ISequencedDocumentMessage.clientId type is wrong
- AB#1387: Drafts: quorum ops seem to be dropped
- AB#1385: Follow up to message refactoring work
Looking for feedback in this specific area:
- There is lack of symmetry when it comes to processing MessageType.Propose & MessageType.Summarize ops. In both cases, sender (protocol, runtime) provides JSON through dedicated API, i.e., sender does not use specific type of message and Loader layer adds message type and strigifyes content.
- But on receive, protocol & runtime need to know message type and need to know they need to parse contents.
- We can shift strigification to those layers, but this would cause type erasure for summary payload (and extra parsing/stringification) and does not solve summary type problem.