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

POC to discuss: Add create/load Container APIs that give you an object before doing full initialization #23709

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
+150 −60
Draft

POC to discuss: Add create/load Container APIs that give you an object before doing full initialization #23709

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
f1a9ced
First try at new API
markfields Jan 31, 2025
440a313
Put initialize on Container, returning Promise<void>
markfields Jan 31, 2025
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
133 changes: 78 additions & 55 deletions packages/loader/container-loader/src/container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,81 +379,104 @@ export class Container
extends EventEmitterWithErrorHandling<IContainerEvents>
implements IContainer, IContainerExperimental
{
/**
* Not to be called directly on this type.
*/
public async initialize(): Promise<void> {
assert(false, "Method will be provided based on create/load API that is used");
Copy link
Contributor

Choose a reason for hiding this comment

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

The assert text is hard to parse - it becomes clearer what it means only by reading the rest of the code.
I think we can avoid the need to have it, and the need to expose container in weird states.

How about the following:

  • create() -> ContainerShell, can register on events only and has only one method:
    • ContainerShell.start() -> Container

This clearly requires proxying event registrations from shell to real container underneath.
But this kind of structure completely eliminates the risk of calling some public APIs on Container while it's not initialized. It also completely separates the workflows, allowing to remove APIs from Container (like initialize(), maybe others) that should not be there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Problem I faced with that is, do you also keep the shell around and continue to use it for events? It will actually be the same instance as the container returned by start.

}

/**
* Load an existing container.
*/
public static async load(
loadProps: IContainerLoadProps,
createProps: IContainerCreateProps,
): Promise<Container> {
const container = Container.loadUninitialized(loadProps, createProps);
return container.initialize().then(() => container);
}

/**
* Load an existing container.
*/
public static loadUninitialized(
loadProps: IContainerLoadProps,
createProps: IContainerCreateProps,
): Container & { initialize: () => Promise<void> } {
const { version, pendingLocalState, loadMode, resolvedUrl } = loadProps;

const container = new Container(createProps, loadProps);

return PerformanceEvent.timedExecAsync(
container.mc.logger,
{ eventName: "Load", ...loadMode },
async (event) =>
new Promise<Container>((resolve, reject) => {
const defaultMode: IContainerLoadMode = { opsBeforeReturn: "cached" };
// if we have pendingLocalState, anything we cached is not useful and we shouldn't wait for connection
// to return container, so ignore this value and use undefined for opsBeforeReturn
const mode: IContainerLoadMode = pendingLocalState
? { ...(loadMode ?? defaultMode), opsBeforeReturn: undefined }
: (loadMode ?? defaultMode);

const onClosed = (err?: ICriticalContainerError): void => {
// pre-0.58 error message: containerClosedWithoutErrorDuringLoad
reject(err ?? new GenericError("Container closed without error during load"));
};
container.on("closed", onClosed);

container
.load(version, mode, resolvedUrl, pendingLocalState)
.finally(() => {
container.removeListener("closed", onClosed);
})
.then(
(props) => {
event.end({ ...props });
resolve(container);
},
(error) => {
const err = normalizeError(error);
// Depending where error happens, we can be attempting to connect to web socket
// and continuously retrying (consider offline mode)
// Host has no container to close, so it's prudent to do it here
// Note: We could only dispose the container instead of just close but that would
// the telemetry where users sometimes search for ContainerClose event to look
// for load failures. So not removing this at this time.
container.close(err);
container.dispose(err);
onClosed(err);
},
);
}),
{ start: true, end: true, cancel: "generic" },
);
const initialize = async (): Promise<void> =>
PerformanceEvent.timedExecAsync(
container.mc.logger,
{ eventName: "Load", ...loadMode },
async (event) =>
new Promise((resolve, reject) => {
const defaultMode: IContainerLoadMode = { opsBeforeReturn: "cached" };
// if we have pendingLocalState, anything we cached is not useful and we shouldn't wait for connection
// to return container, so ignore this value and use undefined for opsBeforeReturn
const mode: IContainerLoadMode = pendingLocalState
? { ...(loadMode ?? defaultMode), opsBeforeReturn: undefined }
: (loadMode ?? defaultMode);

const onClosed = (err?: ICriticalContainerError): void => {
// pre-0.58 error message: containerClosedWithoutErrorDuringLoad
reject(err ?? new GenericError("Container closed without error during load"));
};
container.on("closed", onClosed);

container
.load(version, mode, resolvedUrl, pendingLocalState)
.finally(() => {
container.removeListener("closed", onClosed);
})
.then(
(props) => {
event.end({ ...props });
resolve();
},
(error) => {
const err = normalizeError(error);
// Depending where error happens, we can be attempting to connect to web socket
// and continuously retrying (consider offline mode)
// Host has no container to close, so it's prudent to do it here
// Note: We could only dispose the container instead of just close but that would
// the telemetry where users sometimes search for ContainerClose event to look
// for load failures. So not removing this at this time.
container.close(err);
container.dispose(err);
onClosed(err);
},
);
}),
{ start: true, end: true, cancel: "generic" },
);

return Object.assign(container, { initialize });
Copy link
Contributor

@anthony-murphy anthony-murphy Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

i really don't like the use of object.assign in this pr. ideally we can decompose whatever is happening into free functions and small objects to build up the container, rather than tacking new functions on top the Container monolith

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I don't either, just playing around. I struggle to communicate what I'm looking for feedback on sometimes with these exploratory PRs.

}

/**
* Create a new container in a detached state.
*/
public static async createDetached(
public static createDetached(
createProps: IContainerCreateProps,
codeDetails: IFluidCodeDetails,
): Promise<Container> {
): Container & { initialize: () => Promise<void> } {
Copy link
Contributor

Choose a reason for hiding this comment

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

Container type already has initialize on it - why do you need this new type (that looks identical to Container)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, lots to clean up in implementation, looking for feedback on the API first

Copy link
Contributor

Choose a reason for hiding this comment

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

i find api easiest to review in the api reports, so you should update/generate those

const container = new Container(createProps);

return PerformanceEvent.timedExecAsync(
container.mc.logger,
{ eventName: "CreateDetached" },
async (_event) => {
await container.createDetached(codeDetails);
return container;
},
{ start: true, end: true, cancel: "generic" },
);
return Object.assign(container, {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I got it right, you want symmetrical flow, i.e. call initialize() in both cases.
I'm not sure I like it - while symmetry is nice, it exposes container to users in a new state, which means we now need to test all the things that user can do with such container in this state (before initialize() is called), even if the message is - do not call any API. I'd prefer the flow I described above, and leave detached flow as is, i.e. single call returns detached container.

initialize: async () =>
PerformanceEvent.timedExecAsync(
container.mc.logger,
{ eventName: "CreateDetached" },
async (_event) => {
await container.createDetached(codeDetails);
},
{ start: true, end: true, cancel: "generic" },
),
});
}

/**
Expand Down
37 changes: 37 additions & 0 deletions packages/loader/container-loader/src/createAndLoadContainerUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,27 @@ export async function createDetachedContainer(
});
}

/**
* Creates a new container using the specified code details but in an unattached state. While unattached, all
* updates will only be local until the user explicitly attaches the container to a service provider.
* @param createDetachedContainerProps - Services and properties necessary for creating detached container.
* @legacy
* @alpha
*/
export function createDetachedContainerUninitialized(
createDetachedContainerProps: ICreateDetachedContainerProps,
): IContainer & { initialize: () => Promise<void> } {
const loader = new Loader(createDetachedContainerProps);
const container = loader.createDetachedContainerUninitialized(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd find a way not to use Loader in these new workflows. I.e. refactor code enough where loader is not present in this flow (calling APIs that came out of loader is Ok).
Reason being - I'd rather do all the required refactoring right now, potentially taking some risk introducing bugs into this new workflow right now, not later. They will be easier to discover, as clients would be changing their code, and thus putting more scrutiny to test it at that moment. If we do behind the scenes refactoring later, we have higher chance of introducing and not noticing bugs.
That said, the assumption I'm making here - that we will simplify this new workflow, removing (not calling) some functionality on this new path (thus making it more risky). Maybe not a good example (as I think we already disabled it), but if Container caching was still On, I'd expect this new path to stop caching container, while leaving caching on legacy flow. Same for any other functionality that we see in Loader flow that we do not want to see going forward.

createDetachedContainerProps.codeDetails,
{
canReconnect: createDetachedContainerProps.allowReconnect,
clientDetailsOverride: createDetachedContainerProps.clientDetailsOverride,
},
);
return container;
}

/**
* Creates a new container using the specified snapshot but in an unattached state. While unattached, all
* updates will only be local until the user explicitly attaches the container to a service provider.
Expand Down Expand Up @@ -180,3 +201,19 @@ export async function loadExistingContainer(
loadExistingContainerProps.pendingLocalState,
);
}

/**
* Loads a container with an existing snapshot from the service.
* @param loadExistingContainerProps - Services and properties necessary for loading an existing container.
* @legacy
* @alpha
*/
export async function loadExistingContainerUninitialized(
loadExistingContainerProps: ILoadExistingContainerProps,
): Promise<IContainer & { initialize: () => Promise<void> }> {
const loader = new Loader(loadExistingContainerProps);
return loader.resolveUninitialized(
loadExistingContainerProps.request,
loadExistingContainerProps.pendingLocalState,
);
}
40 changes: 35 additions & 5 deletions packages/loader/container-loader/src/loader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,21 @@ export class Loader implements IHostLoader {
clientDetailsOverride?: IClientDetails;
},
): Promise<IContainer> {
const container = this.createDetachedContainerUninitialized(
codeDetails,
createDetachedProps,
);
//*
return container.initialize().then(() => container);
}

public createDetachedContainerUninitialized(
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be moving away from the loader, so i wouldn't expect any changes in the loader, the functions should do all the work

codeDetails: IFluidCodeDetails,
createDetachedProps?: {
canReconnect?: boolean;
clientDetailsOverride?: IClientDetails;
},
): Container & { initialize: () => Promise<void> } {
return Container.createDetached(
{
...createDetachedProps,
Expand All @@ -362,6 +377,21 @@ export class Loader implements IHostLoader {
}

public async resolve(request: IRequest, pendingLocalState?: string): Promise<IContainer> {
const eventName = pendingLocalState === undefined ? "Resolve" : "ResolveWithPendingState";
return PerformanceEvent.timedExecAsync(this.mc.logger, { eventName }, async () => {
const container = await this.resolveCore(
request,
getAttachedContainerStateFromSerializedContainer(pendingLocalState),
);
await container.initialize();
return container;
});
}

public async resolveUninitialized(
request: IRequest,
pendingLocalState?: string,
): Promise<Container & { initialize: () => Promise<void> }> {
const eventName = pendingLocalState === undefined ? "Resolve" : "ResolveWithPendingState";
return PerformanceEvent.timedExecAsync(this.mc.logger, { eventName }, async () => {
return this.resolveCore(
Expand All @@ -374,7 +404,7 @@ export class Loader implements IHostLoader {
private async resolveCore(
request: IRequest,
pendingLocalState?: IPendingContainerState,
): Promise<Container> {
): Promise<Container & { initialize: () => Promise<void> }> {
const resolvedAsFluid = await this.services.urlResolver.resolve(request);
ensureResolvedUrlDefined(resolvedAsFluid);

Expand All @@ -401,15 +431,15 @@ export class Loader implements IHostLoader {
request.headers[LoaderHeader.version] =
parsed.version ?? request.headers[LoaderHeader.version];

return this.loadContainer(request, resolvedAsFluid, pendingLocalState);
return this.loadContainerUninitialized(request, resolvedAsFluid, pendingLocalState);
}

private async loadContainer(
private loadContainerUninitialized(
request: IRequest,
resolvedUrl: IResolvedUrl,
pendingLocalState?: IPendingContainerState,
): Promise<Container> {
return Container.load(
): Container & { initialize: () => Promise<void> } {
return Container.loadUninitialized(
{
resolvedUrl,
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
Expand Down
Loading