-
Notifications
You must be signed in to change notification settings - Fork 576
feat(container-loader): add readOnly option to loadFrozenContainerFromPendingState #27211
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.
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
base: main
Are you sure you want to change the base?
Changes from 15 commits
3d57157
2991cf6
8f2774f
e771980
fb6a0fd
f4a202e
a08917f
3e767b8
7c31d95
ff37f19
411863f
d1fd645
7866ef5
cdcf203
849c0ed
ce7f4ed
e93eb26
ac44bd1
ebb5b29
8f15eef
86c509a
2035078
4ddd5c1
a3360ee
102c584
87fe785
dfcb644
78c9ef4
731aaea
e8dd9b9
7422e30
7312def
858990e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| --- | ||
| "@fluidframework/container-loader": minor | ||
| "__section": feature | ||
| --- | ||
| Add readOnly option to loadFrozenContainerFromPendingState | ||
|
Check failure on line 5 in .changeset/frozen-container-readonly-option.md
|
||
|
|
||
| `loadFrozenContainerFromPendingState` and `createFrozenDocumentServiceFactory` now accept an optional `readOnly` parameter (default `true`, preserving existing behavior). | ||
|
|
||
| When `readOnly: false`, the frozen container loads as writable so the runtime accepts DDS submissions. The container stays `Connected` against the synthetic `FrozenDeltaStream`: `ConnectionManager.sendMessages` recognizes it as the live connection and short-circuits before the read-mode reconnect branch, dropping outbound messages at the connection-manager layer. Submitted ops accumulate in the runtime's pending-state manager so `getPendingLocalState()` can capture them. | ||
|
Check failure on line 9 in .changeset/frozen-container-readonly-option.md
|
||
|
|
||
| Use `readOnly: false` when the caller wants to load a frozen container, apply additional local changes, and capture the resulting pending state via `getPendingLocalState()`. | ||
|
|
||
| Also: `FrozenDeltaStream.submitSignal` is now a silent no-op for both variants. The pre-existing read-only-variant behavior was a 403 nack on signals; this PR drops it for both variants because (a) for the writable variant a stray signal would close or reconnect the container, and (b) signals are ephemeral and dropping them is the correct behavior with no upstream. `FrozenDeltaStream.submit` continues to nack defensively. | ||
|
Check failure on line 13 in .changeset/frozen-container-readonly-option.md
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -582,9 +582,9 @@ export class ConnectionManager implements IConnectionManager { | |
| LogLevel.verbose, | ||
| ); | ||
| if (isDeltaStreamConnectionForbiddenError(origError)) { | ||
| connection = new FrozenDeltaStream(origError.storageOnlyReason, { | ||
| text: origError.message, | ||
| error: origError, | ||
| connection = new FrozenDeltaStream({ | ||
| storageOnlyReason: origError.storageOnlyReason, | ||
| readonlyConnectionReason: { text: origError.message, error: origError }, | ||
| }); | ||
| requestedMode = "read"; | ||
| break; | ||
|
|
@@ -594,9 +594,8 @@ export class ConnectionManager implements IConnectionManager { | |
| ) { | ||
| // If we get out of storage error from calling joinsession, then use the NoDeltaStream object so | ||
|
markfields marked this conversation as resolved.
Outdated
|
||
| // that user can at least load the container. | ||
| connection = new FrozenDeltaStream(undefined, { | ||
| text: origError.message, | ||
| error: origError, | ||
| connection = new FrozenDeltaStream({ | ||
| readonlyConnectionReason: { text: origError.message, error: origError }, | ||
| }); | ||
| requestedMode = "read"; | ||
| break; | ||
|
|
@@ -1089,6 +1088,21 @@ export class ConnectionManager implements IConnectionManager { | |
|
|
||
| public sendMessages(messages: IDocumentMessage[]): void { | ||
| assert(this.connected, 0x2b4 /* "not connected on sending ops!" */); | ||
|
anthony-murphy marked this conversation as resolved.
|
||
| // FrozenDeltaStream short-circuit: writable-frozen containers | ||
| // (`loadFrozenContainerFromPendingState({ readOnly: false })`) attach a | ||
| // FrozenDeltaStream as the live connection. Its `mode` is "read" (advertising | ||
| // "write" would imply quorum membership we cannot honor), so a runtime submit | ||
| // would otherwise fall into the read-mode reconnect branch below. That branch | ||
| // schedules `reconnect("write")`, which under `ReconnectMode.Never` | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if there are other cases in the runtime that trigger reconnect that would cause trouble?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Walked through the runtime-reconnect triggers — they all funnel into
So the only path that would still trigger a runtime-driven write reconnect is a stray submit hitting the connection's Are you thinking of a specific scenario I haven't covered? Happy to add a test if there's a path I'm missing. |
||
| // (`allowReconnect: false`) calls `closeHandler` and closes the container — the | ||
| // opposite of what writable-frozen wants. Drop the messages here: the runtime's | ||
| // outbox keeps them in `pendingStateManager` so `getPendingLocalState()` can | ||
| // capture them, which is the entire point of the writable-frozen flow. The | ||
| // read-only variant should never reach here (its `storageOnly` policy keeps the | ||
| // runtime from submitting), but covering it too is harmless defense-in-depth. | ||
| if (this.connection instanceof FrozenDeltaStream) { | ||
| return; | ||
| } | ||
| // If connection is "read" or implicit "read" (got leave op for "write" connection), | ||
| // then op can't make it through - we will get a nack if op is sent. | ||
| // We can short-circuit this process. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -229,6 +229,28 @@ export interface ILoadFrozenContainerFromPendingStateProps | |
| * Pending local state to be applied to the container. | ||
| */ | ||
| readonly pendingLocalState: string; | ||
|
|
||
| /** | ||
| * Controls whether the frozen container is surfaced as read-only. | ||
| * | ||
| * Defaults to `true`. When `true`, the container reports `readOnlyInfo.readonly === true` | ||
| * with `storageOnly === true`, matching the historical behavior of frozen loads. | ||
| * | ||
| * When `false`, the container loads as writable so the runtime will accept DDS submissions. | ||
| * The connection itself stays `Connected`: the connection manager recognizes the synthetic | ||
| * frozen delta stream and drops outbound messages at the network layer, so no read→write | ||
| * reconnect is attempted. Local DDS state continues to update via optimistic apply, and | ||
| * submitted ops accumulate in the runtime's pending-state manager. Use this when callers | ||
| * want to accrue and capture pending state without publishing it. | ||
| * | ||
| * @remarks | ||
| * The flag uses negative polarity (`readOnly`) rather than a positive opt-in (`writable`) | ||
| * to align with `IContainer.readOnlyInfo.readonly`, which is the established surface for | ||
| * read/write state on a loaded container. A future positive-polarity option can layer on | ||
| * top of this without breaking callers, but flipping the polarity now would split readers | ||
| * between two conventions for the same concept. | ||
| */ | ||
| readonly readOnly?: boolean; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The term "readonly" typically makes me think of a state where you can't write but you can receive remote changes. I'd call the parameter "allowLocalChanges" (defaulting to false).
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or if you want negative polarity (I just read the remarks), then
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Went with positive
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On reflection, going back to The |
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -241,7 +263,10 @@ export async function loadFrozenContainerFromPendingState( | |
| ): Promise<IContainer> { | ||
| return loadExistingContainer({ | ||
| ...props, | ||
| documentServiceFactory: createFrozenDocumentServiceFactory(props.documentServiceFactory), | ||
| documentServiceFactory: createFrozenDocumentServiceFactory( | ||
| props.documentServiceFactory, | ||
| props.readOnly, | ||
| ), | ||
| }); | ||
| } | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.