-
Notifications
You must be signed in to change notification settings - Fork 532
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
Enable Synchronous Child Datastore Creation #22962
base: main
Are you sure you want to change the base?
Changes from all commits
5927f19
9125154
2769375
7fc16d8
644e1fb
4e933a0
c5b3581
67a245d
08cb956
85369a1
2df41d3
e569174
42ffd71
e19241d
bb44f76
33a08d7
24a7d4d
908b467
d6ad348
226fd1d
810fc0c
cde8b76
f9d671a
da3361e
fb46878
50325b2
ee11048
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,172 @@ | ||
/*! | ||
* Copyright (c) Microsoft Corporation and contributors. All rights reserved. | ||
* Licensed under the MIT License. | ||
*/ | ||
|
||
import { strict as assert } from "assert"; | ||
|
||
import { FluidErrorTypes } from "@fluidframework/core-interfaces/internal"; | ||
import { LazyPromise } from "@fluidframework/core-utils/internal"; | ||
import { IDocumentStorageService } from "@fluidframework/driver-definitions/internal"; | ||
import { | ||
IFluidDataStoreChannel, | ||
IFluidDataStoreFactory, | ||
IFluidDataStoreRegistry, | ||
IFluidParentContext, | ||
type NamedFluidDataStoreRegistryEntries, | ||
type IContainerRuntimeBase, | ||
type ISummarizerNodeWithGC, | ||
} from "@fluidframework/runtime-definitions/internal"; | ||
import { isFluidError } from "@fluidframework/telemetry-utils/internal"; | ||
import { MockFluidDataStoreRuntime } from "@fluidframework/test-runtime-utils/internal"; | ||
|
||
import { | ||
FluidDataStoreContext, | ||
LocalDetachedFluidDataStoreContext, | ||
} from "../dataStoreContext.js"; | ||
|
||
describe("createChildDataStoreSync", () => { | ||
const throwNYI = () => { | ||
throw new Error("Method not implemented."); | ||
}; | ||
const testContext = class TestContext extends FluidDataStoreContext { | ||
protected pkg = ["ParentDataStore"]; | ||
public registry: IFluidDataStoreRegistry | undefined; | ||
public getInitialSnapshotDetails = throwNYI; | ||
public setAttachState = throwNYI; | ||
public getAttachSummary = throwNYI; | ||
public getAttachGCData = throwNYI; | ||
protected channel = new Proxy({} as any as IFluidDataStoreChannel, { get: throwNYI }); | ||
protected channelP = new LazyPromise(async () => this.channel); | ||
}; | ||
|
||
const createRegistry = ( | ||
namedEntries?: NamedFluidDataStoreRegistryEntries, | ||
): IFluidDataStoreRegistry => ({ | ||
get IFluidDataStoreRegistry() { | ||
return this; | ||
}, | ||
// eslint-disable-next-line @typescript-eslint/promise-function-async | ||
get(name) { | ||
return new Map(namedEntries).get(name); | ||
}, | ||
}); | ||
|
||
const createContext = (namedEntries?: NamedFluidDataStoreRegistryEntries) => { | ||
const registry = createRegistry(namedEntries); | ||
const createSummarizerNodeFn = () => | ||
new Proxy({} as any as ISummarizerNodeWithGC, { get: throwNYI }); | ||
const storage = new Proxy({} as any as IDocumentStorageService, { get: throwNYI }); | ||
|
||
const parentContext = { | ||
clientDetails: { | ||
capabilities: { interactive: true }, | ||
}, | ||
containerRuntime: { | ||
createDetachedDataStore(pkg, loadingGroupId) { | ||
return new LocalDetachedFluidDataStoreContext({ | ||
channelToDataStoreFn: (channel) => ({ | ||
entryPoint: channel.entryPoint, | ||
trySetAlias: throwNYI, | ||
}), | ||
createSummarizerNodeFn, | ||
id: "child", | ||
makeLocallyVisibleFn: throwNYI, | ||
parentContext, | ||
pkg, | ||
scope: {}, | ||
snapshotTree: undefined, | ||
storage, | ||
loadingGroupId, | ||
}); | ||
}, | ||
} satisfies Partial<IContainerRuntimeBase> as unknown as IContainerRuntimeBase, | ||
} satisfies Partial<IFluidParentContext> as unknown as IFluidParentContext; | ||
|
||
const context = new testContext( | ||
{ | ||
createSummarizerNodeFn, | ||
id: "parent", | ||
parentContext, | ||
scope: {}, | ||
storage, | ||
}, | ||
false, | ||
false, | ||
throwNYI, | ||
); | ||
context.registry = registry; | ||
return context; | ||
}; | ||
|
||
const createFactory = ( | ||
createDataStore?: IFluidDataStoreFactory["createDataStore"], | ||
): IFluidDataStoreFactory => ({ | ||
type: "ChildDataStore", | ||
get IFluidDataStoreFactory() { | ||
return this; | ||
}, | ||
instantiateDataStore: throwNYI, | ||
createDataStore, | ||
}); | ||
|
||
it("Child factory does not support synchronous creation", async () => { | ||
const factory = createFactory(); | ||
const context = createContext([[factory.type, factory]]); | ||
try { | ||
context.createChildDataStoreSync(factory); | ||
assert.fail("should fail"); | ||
} catch (e) { | ||
assert(isFluidError(e)); | ||
assert(e.errorType === FluidErrorTypes.usageError); | ||
assert(e.getTelemetryProperties().noCreateDataStore === true); | ||
} | ||
}); | ||
|
||
it("Child factory not registered", async () => { | ||
const factory = createFactory(); | ||
const context = createContext(); | ||
try { | ||
context.createChildDataStoreSync(factory); | ||
assert.fail("should fail"); | ||
} catch (e) { | ||
assert(isFluidError(e)); | ||
assert(e.errorType === FluidErrorTypes.usageError); | ||
assert(e.getTelemetryProperties().isUndefined === true); | ||
} | ||
}); | ||
|
||
it("Child factory is a promise", async () => { | ||
const factory = createFactory(); | ||
const context = createContext([[factory.type, Promise.resolve(factory)]]); | ||
|
||
try { | ||
context.createChildDataStoreSync(factory); | ||
assert.fail("should fail"); | ||
} catch (e) { | ||
assert(isFluidError(e)); | ||
assert(e.errorType === FluidErrorTypes.usageError); | ||
assert(e.getTelemetryProperties().isPromise === true); | ||
} | ||
}); | ||
|
||
it("Child factory is a different instance", async () => { | ||
const factory = createFactory(); | ||
const context = createContext([[factory.type, createFactory()]]); | ||
|
||
try { | ||
context.createChildDataStoreSync(factory); | ||
assert.fail("should fail"); | ||
} catch (e) { | ||
assert(isFluidError(e)); | ||
assert(e.errorType === FluidErrorTypes.usageError); | ||
assert(e.getTelemetryProperties().diffInstance === true); | ||
} | ||
}); | ||
|
||
it("createChildDataStoreSync", async () => { | ||
const factory = createFactory(() => ({ runtime: new MockFluidDataStoreRuntime() })); | ||
const context = createContext([[factory.type, factory]]); | ||
context.createChildDataStoreSync(factory); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,7 +28,10 @@ import type { | |
} from "@fluidframework/driver-definitions/internal"; | ||
import type { IIdCompressor } from "@fluidframework/id-compressor"; | ||
|
||
import type { IProvideFluidDataStoreFactory } from "./dataStoreFactory.js"; | ||
import type { | ||
IFluidDataStoreFactory, | ||
IProvideFluidDataStoreFactory, | ||
} from "./dataStoreFactory.js"; | ||
import type { IProvideFluidDataStoreRegistry } from "./dataStoreRegistry.js"; | ||
import type { | ||
IGarbageCollectionData, | ||
|
@@ -610,6 +613,28 @@ export interface IFluidDataStoreContext extends IFluidParentContext { | |
* and its children with the GC details from the previous summary. | ||
*/ | ||
getBaseGCDetails(): Promise<IGarbageCollectionDetailsBase>; | ||
|
||
/** | ||
* This function creates a detached child data store synchronously. | ||
* | ||
* The `createChildDataStoreSync` method allows for the synchronous creation of a child data store. This is particularly | ||
* useful in scenarios where immediate availability of the child data store is required, such as during the initialization | ||
* of a parent data store, or when creation is in response to synchronous user input. | ||
* | ||
* In order for this function to succeed: | ||
* 1. The parent data store's factory must also be an `IFluidDataStoreRegistry`. | ||
* 2. The parent data store's registry must include the same instance as the provided child factory. | ||
* 3. The parent data store's registry must synchronously provide the child factory. | ||
* 4. The child factory must implement the `createDataStore` method. | ||
* | ||
* These invariants ensure that the child data store can also be created by a remote client running the same code as this client. | ||
* | ||
* @param childFactory - The factory of the data store to be created. | ||
* @returns The created data store channel. | ||
*/ | ||
createChildDataStoreSync?<T extends IFluidDataStoreFactory>( | ||
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. EDIT: I meant to put this comment in dataStoreFactory.ts, oops :) I don't think I understand what the motivating scenario is for this PR, so I'd love to learn more about that. This comment is purely taking this PR at face value, so if I'm missing some special context let me know. At a very high level, I'd question whether a sync factory pattern is moving in a good direction for our overall instantiation flows. One pain point of Aqueduct (and therefore almost all of our current data store examples) is that the async startup flows are deferred to the data store instance (i.e. async Fluid basically mandates async somewhere in the construction flow though, due to retrieving handles, getChannel, etc.), so we can't go fully sync all the way to full initialization for most scenarios. An alternative approach (ditching Aqueduct) would be to embrace an async factory, and move all async initialization up to the factory such that by the time we call the instance constructor, we are ready to be fully sync (and the instance is fully initialized in the constructor). I've explored this pattern in https://github.com/microsoft/FluidFramework/blob/main/examples/utils/migration-tools/src/migrationTool.ts if you'd like an example. Fully admit there's some personal preference here, but I think this aligns better with DI principles, simplifies the instance class, and strengthens the instance class's invariants. 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. scriptor has a case where they want to create a child datastore in response to user input, so there is no opportunity to do async actions. i'd not confuse this with all instantiation, which include load, load must always be async. Create on the other hand can be synchronous, and that is what is enabled here. This matches how dds work. We can't split the sync part and the async part here. the whole things need to be sync, as we need a net new datastore synchronously that ready to be used, and have its handle stores in the same js turn, to not break on user input. 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 see, thank you - that makes more sense. |
||
childFactory: T, | ||
): ReturnType<Exclude<T["createDataStore"], undefined>>; | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reasoning for:
createDataStore
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only the context has access to its factory synchronously, as its factory must be loaded before it is realized. This is basically the same pattern we use for dds.