-
Notifications
You must be signed in to change notification settings - Fork 0
[Prototype] Introducing a new "attach broker" node for managing the bind/attach graph #6
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
Conversation
|
markfields: Tell me what you see about how we use handle binding to determine when to attach DDSes (aka SharedObjects) and DataStores GitHub Copilot: Based on the provided code:
In summary, handle binding ( markfields: Ok cool that sounds about right. I'd like to introduce a new structure to abstract this away from handles. Let's create a type ChannelAttachBroker. The FluidDataStoreRuntime will have a broker for itself and each of its child channels. ChannelAttachBrokers will form the reference graph used to determine when to attach the nodes they track. A ChannelAttachBroker does the following:
When an outbound handle from DDS A to target object B is found via serialization, add B's broker to A's broker's list of referenced brokers. To do this we'll have to update IFluidHandleInternal to have a broker. Adding this reference also needs to trigger a calculation of whether the attach state of any subgraph changed. I think we can do this by having the DataStore listen to an event that is fired when the broker adds an outbound reference. When this happens the DataStore checks if the source broker is attached, and if so, it calls the attach fn on the target broker. Can you please implement ChannelAttachBroker and update FluidSerializer.bindAndEncodeHandle to use brokers instead of calling IFluidHandleInternal.bind? GitHub Copilot: Okay, let's introduce the
Made changes. Made changes. Made changes. Made changes. OK. I've created the I've also added the Finally, I updated the However, there are several compilation errors resulting from these changes, primarily related to module imports, interface augmentations, and missing properties/methods that the new broker classes rely on. Let's fix these errors. Made changes. Made changes. Made changes. Made changes. Okay, I can help with that. I will create the
Made changes. Made changes. Made changes. Made changes. Made changes. Made changes. Made changes. Made changes. |
|
markfields: In this file, switch broker to be passed in directly via constructor rather than fetched off this.value GitHub Copilot: Okay, I will modify the First, I'll apply the changes to fluidHandle.ts. Made changes. Okay, I've refactored Now I need to update the places where
I'll read these files and update the constructor calls.Okay, I have refactored Looking at the usages of Additionally, I'll apply these changes now. Made changes. Okay, I've updated the Based on the usage information, I'll modify handle.ts to pass the broker in the The usage in dataStoreRuntime.ts involves creating a handle for a Updating handle.ts: Made changes. Now I need to find other places where The usage of Now, I'll check for any errors introduced by these changes. In handle.ts:
In fluidHandle.ts:
Made changes. Okay, I have fixed the import errors in handle.ts. Now, I will address the remaining errors in that file by removing the overridden Made changes. I have updated the I'll check for any remaining errors in the modified files.Okay, I see several errors reported after the initial changes. I'll fix them now. First, I'll fix the import issues, remove the unnecessary Made changes. I have updated the I also updated the Finally, I fixed the type errors and lint issues that arose from these changes. |
|
markfields: Add an interface IChannelAttachBroker, based on the class ChannelAttachBroker GitHub Copilot: Okay, I can help with that. Here's the plan:
handles.tsDefine // ...existing code...
/**
* Describes a routing context from which other `IFluidHandleContext`s are defined.
* @legacy
* @alpha
*/
export interface IFluidHandleContext extends IProvideFluidHandleContext {
// ...existing code...
}
/**
* Interface for managing the attachment state of an object associated with a handle.
* @internal
*/
export interface IChannelAttachBroker {
/**
* Flag indicating whether or not the associated object is attached.
*/
readonly isAttached: boolean;
// TODO: Add other relevant methods/properties as needed, e.g., for attach propagation or reference tracking.
}
/**
* @public
* @privateRemarks
// ...existing code...
*/
bind(handle: IFluidHandleInternal): void;
/**
* The broker responsible for managing the attachment state of the referenced object.
*/
readonly broker: IChannelAttachBroker;
}
/**
* Symbol which must only be used on an {@link (IFluidHandle:interface)}, and is used to identify such objects.
// ...existing code...Made changes. markfields: Move channelAttachBroker.ts to the runtime-utils package GitHub Copilot: Okay, I will move the channelAttachBroker.tsCreate the new file with the existing content. /*!
* Copyright (c) Microsoft Corporation and contributors. All rights reserved.
* Licensed under the MIT License.
*/
import { TypedEventEmitter } from "@fluid-internal/client-utils";
import type {
IChannel,
IFluidDataStoreRuntime,
} from "@fluidframework/datastore-definitions/internal";
import type { IChannelAttachBroker } from "@fluidframework/core-interfaces/internal";
/**
* Events emitted by a ChannelAttachBroker.
* @internal
*/
export type IChannelAttachBrokerEvents =
/**
* Emitted when a new outbound reference is added from this broker to a target broker.
* This happens during serialization when a handle is encoded.
*/
(event: "referenceAdded", listener: (targetBroker: ChannelAttachBroker) => void) => void;
/**
* Manages the attachment relationship and propagation for a specific channel (DDS or DataStore).
* Brokers form a graph based on references (e.g., storing a handle). When a broker becomes attached,
* it automatically triggers the attachment of brokers it references, if they are not already attached.
* @internal
*/
export class ChannelAttachBroker
extends TypedEventEmitter<IChannelAttachBrokerEvents>
implements IChannelAttachBroker
{
private _isAttached: boolean;
public readonly referencedBrokers = new Set<ChannelAttachBroker>();
/**
* Creates an instance of ChannelAttachBroker.
* @param object - The channel (DDS or DataStore) this broker is associated with.
* @param containingDataStore - The runtime of the data store that contains this broker's channel.
* @param attachFn - The function to call to physically attach the associated channel.
* This function is responsible for eventually calling `setAttached()` on this broker.
* @param initialAttachedState - The initial attachment state of the channel.
*/
constructor(
public readonly object: IChannel, // TODO: Consider a more specific type? IFluidLoadable?
public readonly containingDataStore: IFluidDataStoreRuntime,
private readonly attachFn: (sourceDataStore: IFluidDataStoreRuntime) => void,
initialAttachedState: boolean,
) {
super();
this._isAttached = initialAttachedState;
}
/**
* Indicates whether the channel associated with this broker is considered attached.
*/
public get isAttached(): boolean {
return this._isAttached;
}
/**
* Marks the associated channel as attached and propagates attachment to referenced brokers.
* This should be called by the `attachFn` or the channel's attachment logic completion.
*/
public setAttached(): void {
if (!this._isAttached) {
this._isAttached = true;
// When attached, trigger attach on referenced brokers that are not yet attached.
this.referencedBrokers.forEach((targetBroker) => {
//* Let the target early return if it is already attached.
if (!targetBroker.isAttached) {
// Pass the DataStore of the *current* broker as the source
targetBroker.attach(this.containingDataStore);
}
});
}
}
/**
* Initiates the attachment process for the associated channel.
* If the channel is already attached, this is a no-op.
* @param sourceDataStore - The data store runtime associated with the broker that triggered this attachment.
*/
public attach(sourceDataStore: IFluidDataStoreRuntime): void {
if (!this._isAttached) {
// Call the actual attach implementation provided in the constructor.
// It is expected that the attachFn will eventually call setAttached() on this broker.
this.attachFn(sourceDataStore);
}
}
/**
* Adds a reference from this broker's channel to the target broker's channel.
* This is typically called during serialization when a handle is encountered.
* If the current broker is already attached, this will immediately trigger attachment on the target broker.
* @param targetBroker - The broker for the channel being referenced.
*/
public addReference(targetBroker: ChannelAttachBroker): void {
if (!this.referencedBrokers.has(targetBroker)) {
this.referencedBrokers.add(targetBroker);
this.emit("referenceAdded", targetBroker);
// If the source broker (this) is already attached, immediately try to attach the target.
if (this.isAttached && !targetBroker.isAttached) {
// Pass the DataStore of the *source* broker (this) as the source
targetBroker.attach(this.containingDataStore);
}
}
}
}fluidHandle.tsUpdate the import path for Made changes.
// ...existing code...
import { IFluidHandleContext } from "@fluidframework/core-interfaces/internal";
import {
generateHandleContextPath,
FluidHandleBase,
ChannelAttachBroker, // Import from runtime-utils
} from "@fluidframework/runtime-utils/internal";
// Remove the old import: import type { ChannelAttachBroker } from "./channelAttachBroker.js";
/**
* Handle for a shared {@link @fluidframework/core-interfaces#FluidObject}.
* @legacy
// ...existing code...handle.tsUpdate the import path for Made changes.
// ...existing code...
import { IFluidHandleContext } from "@fluidframework/core-interfaces/internal";
import { FluidObjectHandle } from "@fluidframework/datastore/internal";
import { ChannelAttachBroker } from "@fluidframework/runtime-utils/internal"; // Import from runtime-utils
import { ISharedObject } from "./types.js";
/**
// ...existing code...
*/
export class SharedObjectHandle extends FluidObjectHandle<ISharedObject> {
// Remove the explicit broker property as it's handled by the base class now
// broker: ChannelAttachBroker;
/**
* Whether services have been attached for the associated shared object.
*/
public get isAttached(): boolean {
// Access broker via the getter from the base class
return this.broker.isAttached;
}
/**
* Creates a new SharedObjectHandle.
* @param value - The shared object this handle is for.
* @param path - The id of the shared object. It is also the path to this object relative to the routeContext.
* @param routeContext - The parent {@link @fluidframework/core-interfaces#IFluidHandleContext} that has a route
* to this handle.
* @param broker - The broker associated with the shared object.
*/
constructor(
protected readonly value: ISharedObject,
path: string,
routeContext: IFluidHandleContext,
broker: ChannelAttachBroker, // Keep broker parameter
) {
// Pass the broker to the super constructor
super(value, path, routeContext, broker);
}
// ...existing code...
}Finally, you should delete the original file: Made changes. Similar code found with 1 license type |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
This is an idea I've been pondering for a while, tried it out with Copilot's "help". Pretty rough time actually. Going to abandon this branch but posting a PR in case I want to refer back to it ever.
Dropping the Copilot chat(s) below for fun. Trying to learn how to better prompt it. It just kept making way to big of a change.