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.

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

Add connect(), disconnect(), deprecate setAutoReconnect(), resume(), and connected #9439

Merged
merged 18 commits into from
Apr 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
16 changes: 16 additions & 0 deletions BREAKING.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ There are a few steps you can take to write a good change note and avoid needing
- [IDirectory extends IDisposable](#IDirectory-extends-IDisposable)
- [raiseContainerWarning removed from IContainerContext](#raiseContainerWarning-removed-from-IContainerContext)
- [`IContainerRuntimeBase.setFlushMode` is deprecated](#icontainerruntimebasesetflushmode-is-deprecated)
- [connected deprecated from IContainer, IFluidContainer, and FluidContainer](#connected-deprecated-from-IContainer-IFluidContainer-and-FluidContainer)
- [setAutoReconnect and resume deprecated from IContainer and Container](#setAutoReconnect-and-resume-deprecated-from-IContainer-and-Container)
- [IContainer.connect() and IContainer.disconnect() will be made mandatory in future major release](#icontainer-connect-and-icontainer-disconnect-will-be-made-mandatory-in-future-major-release)

### Doing operations not allowed on deleted sub directory
Users will not be allowed to do operations on a deleted directory. Users can subscribe to `disposed` event to know if a sub directory is deleted. Accessing deleted sub directory will throw `UsageError` exception now.
Expand All @@ -34,6 +37,19 @@ IDirectory has started extending IDisposable. This means that users implementing
### `IContainerRuntimeBase.setFlushMode` is deprecated
`IContainerRuntimeBase.setFlushMode` is deprecated and will be removed in a future release. FlushMode will become an immutable property for the container runtime, optionally provided at creation time via the `IContainerRuntimeOptions` interface. See [#9480](https://github.com/microsoft/FluidFramework/issues/9480#issuecomment-1084790977)

### connected deprecated from IContainer, IFluidContainer, and FluidContainer
`connected` has been deprecated from `IContainer`, `IFluidContainer`, and `FluidContainer`. It will be removed in a future major release. Use `connectionState` property on the respective interfaces/classes instead. Please switch to the new APIs as soon as possible, and provide any feedback to the FluidFramework team if necessary.
``` diff
- if (fluidContainer.connected)
+ if (fluidContainer.connectionState === ConnectionState.Connected)
```

### setAutoReconnect and resume deprecated from IContainer and Container
`setAutoReconnect()` and `resume()` have been deprecated from `IContainer` and `Container`. They will be removed in a future major release. Use `connect()` instead of `setAutoReconnect(true)` and `resume()`, and use `disconnect()` instead of `setAutoReconnect(false)`. Note, when using these new functions you will need to ensure that the container is both attached and not closed to prevent an error being thrown. Please switch to the new APIs as soon as possible, and provide any feedback to the FluidFramework team if necessary.

### IContainer.connect() and IContainer.disconnect() will be made mandatory in future major release
In major release 1.0, the optional functions `IContainer.connect()` `IContainer.disconnect()` will be made mandatory functions.

## 0.58 Breaking changes
- [Move IntervalType from merge-tree to sequence package](#Move-IntervalType-from-merge-tree-to-sequence-package)
- [Remove logger property from IContainerContext](#Remove-logger-property-from-IContainerContext)
Expand Down
8 changes: 6 additions & 2 deletions api-report/container-loader.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,16 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i
// (undocumented)
get closeSignal(): AbortSignal;
// (undocumented)
connect(): void;
markfields marked this conversation as resolved.
Show resolved Hide resolved
// (undocumented)
get connected(): boolean;
// (undocumented)
get connectionState(): ConnectionState;
static createDetached(loader: Loader, codeDetails: IFluidCodeDetails): Promise<Container>;
// (undocumented)
get deltaManager(): IDeltaManager<ISequencedDocumentMessage, IDocumentMessage>;
// (undocumented)
disconnect(): void;
forceReadonly(readonly: boolean): void;
// (undocumented)
getAbsoluteUrl(relativeUrl: string): Promise<string | undefined>;
Expand All @@ -100,13 +104,13 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i
request(path: IRequest): Promise<IResponse>;
// (undocumented)
get resolvedUrl(): IResolvedUrl | undefined;
// (undocumented)
// @deprecated
resume(): void;
get scopes(): string[] | undefined;
// (undocumented)
serialize(): string;
get serviceConfiguration(): IClientConfiguration | undefined;
// (undocumented)
// @deprecated
setAutoReconnect(reconnect: boolean): void;
// (undocumented)
get storage(): IDocumentStorageService;
Expand Down
4 changes: 4 additions & 0 deletions api-report/fluid-static.api.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

import { AttachState } from '@fluidframework/container-definitions';
import { BaseContainerRuntimeFactory } from '@fluidframework/aqueduct';
import { ConnectionState } from '@fluidframework/container-definitions';
import { DataObject } from '@fluidframework/aqueduct';
import { IAudience } from '@fluidframework/container-definitions';
import { IChannelFactory } from '@fluidframework/datastore-definitions';
Expand Down Expand Up @@ -42,6 +43,7 @@ export class FluidContainer extends TypedEventEmitter<IFluidContainerEvents> imp
attach(): Promise<string>;
get attachState(): AttachState;
get connected(): boolean;
get connectionState(): ConnectionState;
create<T extends IFluidLoadable>(objectClass: LoadableObjectClass<T>): Promise<T>;
dispose(): void;
get disposed(): boolean;
Expand All @@ -59,7 +61,9 @@ export interface IConnection {
export interface IFluidContainer extends IEventProvider<IFluidContainerEvents> {
attach(): Promise<string>;
readonly attachState: AttachState;
// @deprecated
readonly connected: boolean;
readonly connectionState: ConnectionState;
create<T extends IFluidLoadable>(objectClass: LoadableObjectClass<T>): Promise<T>;
dispose(): void;
readonly disposed: boolean;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,12 @@ export interface IContainer extends IEventProvider<IContainerEvents>, IFluidRout
close(error?: ICriticalContainerError): void;
closeAndGetPendingLocalState(): string;
readonly closed: boolean;
connect?(): void;
// @deprecated
readonly connected: boolean;
readonly connectionState: ConnectionState;
deltaManager: IDeltaManager<ISequencedDocumentMessage, IDocumentMessage>;
disconnect?(): void;
// @alpha
forceReadonly?(readonly: boolean): any;
getAbsoluteUrl(relativeUrl: string): Promise<string | undefined>;
Expand All @@ -148,10 +151,10 @@ export interface IContainer extends IEventProvider<IContainerEvents>, IFluidRout
readonly readOnlyInfo: ReadOnlyInfo;
request(request: IRequest): Promise<IResponse>;
resolvedUrl: IResolvedUrl | undefined;
// @alpha
// @deprecated
resume?(): void;
serialize(): string;
// @alpha
// @deprecated
setAutoReconnect?(reconnect: boolean): void;
}

Expand Down
21 changes: 19 additions & 2 deletions common/lib/container-definitions/src/loader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -252,20 +252,37 @@ export interface IContainer extends IEventProvider<IContainerEvents>, IFluidRout

/**
* Boolean indicating whether the container is currently connected or not
* @deprecated - 0.58, This API will be removed in 1.0
* Check `connectionState === ConnectionState.Connected` instead
* See https://github.com/microsoft/FluidFramework/issues/9167 for context
scottn12 marked this conversation as resolved.
Show resolved Hide resolved
*/
readonly connected: boolean;

/**
* Attempts to connect the container to the delta stream and process ops
*/
connect?(): void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add new APIs on class, propagate and wait some time, and then add them here, but not make them optional?
Alternatively, we can make them non-optional from start - I do not see big problem here. As long as we are adding implementation in parallel, package bump would be noop in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense. I need to create a follow up PR to add connect() and disconnect() to IFluidContainer anyway, so we could just add in the functions to both IContainer and IFluidContainer in one PR.


/**
* Disconnects the container from the delta stream and stops processing ops
*/
disconnect?(): void;

/**
* Dictates whether or not the current container will automatically attempt to reconnect to the delta stream
* after receiving a disconnect event
* @param reconnect - Boolean indicating if reconnect should automatically occur
* @alpha
* @deprecated - 0.58, This API will be removed in 1.0
* Use `connect()` and `disconnect()` instead of `setAutoReconnect(true)` and `setAutoReconnect(false)` respectively
* See https://github.com/microsoft/FluidFramework/issues/9167 for context
*/
setAutoReconnect?(reconnect: boolean): void;

/**
* Have the container attempt to resume processing ops
* @alpha
* @deprecated - 0.58, This API will be removed in 1.0
* Use `connect()` instead
Copy link
Member

Choose a reason for hiding this comment

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

Should we add connect() and disconnect() as optional right away, especially since you're mentioning them here, and to help consumers transition? I don't think adding an optional thing is ever breaking (but could be wrong).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually had them added as optional earlier but changed it after discussion here. To summarize, I will have to add the functions to IFluidContainer in a separate PR anyway, so I am planning to add them to IContainer and IFluidContainer in one PR. This way we won't have to make another PR to change it from optional to mandatory functions.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Feel free to ignore what I'm about to say then :)

In general, I believe having a property absent is completely equivalent to having it present but optional, in terms of compatibility. It's just a hint saying "you may find something here...". So I would think you can and should add the optional functions to both interfaces in this PR.

This way we won't have to make another PR to change it from optional to mandatory

To say the above another way, changing a member from optional to mandatory is equivalent to adding it when it's missing. At least I think so - correct me if I'm wrong, this stuff really bends my brain.

Copy link
Contributor

Choose a reason for hiding this comment

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

@markfields The main problem with going from optional to mandatory I think is that it can still be a breaking change to anyone with an external implementation of the interface, where an implementation/object matching the interface suddenly no longer does so. It sounds like this shouldn't be an issue in this case though, but if there's a breaking change no matter how you slice it there's some appeal to doing just one step instead of two

Copy link
Member

Choose a reason for hiding this comment

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

I'm saying something different, I agree with that for sure.

Going from nothing to optional is never any trouble. Going from optional to required is trouble - it's as much trouble as adding a required thing outright. So might as well add optional earlier than later to give people an easier time onboarding/preparing.

Copy link
Member

Choose a reason for hiding this comment

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

What's the appeal of doing one step instead of two?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @heliocliu and I'm not sure why this was removed from this PR? The goal of this PR is to introduce connect and disconnect at the Container level. Since we are enforcing using IContainer we are telling devs to use APIs as part of depreciation that aren't actually exposed.

Seems like the right step is to add them as optional as part of this PR and include the version in the Breaking.md that we will be making the breaking change. Then make them mandatory in next (or whatever breaking change we decide)

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the appeal of doing one step instead of two?

Fewer steps 🙃

* See https://github.com/microsoft/FluidFramework/issues/9167 for context
*/
resume?(): void;

Expand Down
13 changes: 11 additions & 2 deletions packages/framework/fluid-static/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,15 @@
},
"typeValidation": {
"version": "0.58.3000",
"broken": {}
"broken": {
"0.58.2000": {
"ClassDeclaration_FluidContainer": {
"forwardCompat": false
},
"InterfaceDeclaration_IFluidContainer": {
"forwardCompat": false
}
Comment on lines +75 to +82
Copy link
Contributor Author

@scottn12 scottn12 Apr 1, 2022

Choose a reason for hiding this comment

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

@anthony-murphy @heliocliu, would this be considered a breaking change that needs to be merged into next instead of main? Initially, I thought it would be considered non-breaking to add connectionState to IFluidContainer and FluidContainer, but the new type validation showed that the previous version is not forward compatible. Thoughts on if this is non-breaking and can be included in this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Forward incompatibility (probably) won't cause compile breaks, but you still need to consider if it will cause runtime compatibility breaks. It looks like that won't be the case here since you're not calling into the loader layer changes from other layers, but it does mean you won't be able to use the new apis in main. This is what Vlad was talking about in #9439 (comment) as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this comment was referring to the new connect()/disconnect() functions being added to Container before adding them on IContainer. In this case, connectionState already exists on Container, so it should be able to work in main. Is that correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah that will be fine

}
}
}
}
}
17 changes: 16 additions & 1 deletion packages/framework/fluid-static/src/fluidContainer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import { TypedEventEmitter } from "@fluidframework/common-utils";
import { IFluidLoadable } from "@fluidframework/core-interfaces";
import { IEvent, IEventProvider } from "@fluidframework/common-definitions";
import { AttachState, IContainer } from "@fluidframework/container-definitions";
import { AttachState, IContainer, ConnectionState } from "@fluidframework/container-definitions";
import { LoadableObjectClass, LoadableObjectRecord } from "./types";
import { RootDataObject } from "./rootDataObject";

Expand Down Expand Up @@ -74,9 +74,17 @@ export interface IFluidContainerEvents extends IEvent {
export interface IFluidContainer extends IEventProvider<IFluidContainerEvents> {
/**
* Whether the container is connected to the collaboration session.
* @deprecated - 0.58, This API will be removed in 1.0
* Check `connectionState === ConnectionState.Connected` instead
* See https://github.com/microsoft/FluidFramework/issues/9167 for context
*/
readonly connected: boolean;

/**
* Provides the current connected state of the container
*/
readonly connectionState: ConnectionState;

/**
* A container is considered **dirty** if it has local changes that have not yet been acknowledged by the service.
* You should always check the `isDirty` flag before closing the container or navigating away from the page.
Expand Down Expand Up @@ -184,6 +192,13 @@ export class FluidContainer extends TypedEventEmitter<IFluidContainerEvents> imp
return this.container.connected;
}

/**
* {@inheritDoc IFluidContainer.connectionState}
*/
public get connectionState(): ConnectionState {
scottn12 marked this conversation as resolved.
Show resolved Hide resolved
return this.container.connectionState;
}

/**
* {@inheritDoc IFluidContainer.initialObjects}
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ declare function get_old_ClassDeclaration_FluidContainer():
declare function use_current_ClassDeclaration_FluidContainer(
use: TypeOnly<current.FluidContainer>);
use_current_ClassDeclaration_FluidContainer(
// @ts-expect-error compatibility expected to be broken
get_old_ClassDeclaration_FluidContainer());

/*
Expand Down Expand Up @@ -144,6 +145,7 @@ declare function get_old_InterfaceDeclaration_IFluidContainer():
declare function use_current_InterfaceDeclaration_IFluidContainer(
use: TypeOnly<current.IFluidContainer>);
use_current_InterfaceDeclaration_IFluidContainer(
// @ts-expect-error compatibility expected to be broken
get_old_InterfaceDeclaration_IFluidContainer());

/*
Expand Down
83 changes: 68 additions & 15 deletions packages/loader/container-loader/src/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -397,7 +397,6 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i

private resumedOpProcessingAfterLoad = false;
private firstConnection = true;
private manualReconnectInProgress = false;
private readonly connectionTransitionTimes: number[] = [];
private messageCountAfterDisconnection: number = 0;
private _loadedFromVersion: IVersion | undefined;
Expand Down Expand Up @@ -909,11 +908,31 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i
);
}

/**
* Dictates whether or not the current container will automatically attempt to reconnect to the delta stream
* after receiving a disconnect event
* @param reconnect - Boolean indicating if reconnect should automatically occur
* @deprecated - 0.58, This API will be removed in 1.0
* Use `connect()` and `disconnect()` instead of `setAutoReconnect(true)` and `setAutoReconnect(false)` respectively
* See https://github.com/microsoft/FluidFramework/issues/9167 for context
*/
public setAutoReconnect(reconnect: boolean) {
if (this.closed) {
throw new Error("Attempting to setAutoReconnect() a closed Container");
}

const mode = reconnect ? ReconnectMode.Enabled : ReconnectMode.Disabled;
this.setAutoReconnectInternal(mode);

// If container state is not attached and resumed, then don't connect to delta stream. Also don't set the
// manual reconnection flag to true as we haven't made the initial connection yet.
if (reconnect && this._attachState === AttachState.Attached && this.resumedOpProcessingAfterLoad) {
// Ensure connection to web socket
this.connectToDeltaStream({ reason: "autoReconnect" });
}
}

private setAutoReconnectInternal(mode: ReconnectMode) {
const currentMode = this._deltaManager.connectionManager.reconnectMode;

if (currentMode === mode) {
Expand All @@ -925,27 +944,65 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i
this.setAutoReconnectTime = now;

this.mc.logger.sendTelemetryEvent({
eventName: reconnect ? "AutoReconnectEnabled" : "AutoReconnectDisabled",
eventName: mode === ReconnectMode.Enabled ? "AutoReconnectEnabled" : "AutoReconnectDisabled",
connectionMode: this.connectionMode,
connectionState: ConnectionState[this.connectionState],
duration,
});

this._deltaManager.connectionManager.setAutoReconnect(mode);
}

// If container state is not attached and resumed, then don't connect to delta stream. Also don't set the
// manual reconnection flag to true as we haven't made the initial connection yet.
if (reconnect && this._attachState === AttachState.Attached && this.resumedOpProcessingAfterLoad) {
if (this.connectionState === ConnectionState.Disconnected) {
// Only track this as a manual reconnection if we are truly the ones kicking it off.
this.manualReconnectInProgress = true;
}
public connect() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

These should actually just inherit the docs from the interface.

if (this.closed) {
throw new UsageError(`The Container is closed and cannot be connected`);
}
else if (this._attachState !== AttachState.Attached) {
throw new UsageError(`The Container is not attached and cannot be connected`);
}
else if (!this.connected) {
// Note: no need to fetch ops as we do it preemptively as part of DeltaManager.attachOpHandler().
// If there is gap, we will learn about it once connected, but the gap should be small (if any),
// assuming that connect() is called quickly after initial container boot.
this.connectInternal({ reason: "DocumentConnect", fetchOpsFromStorage: false });
}
}

// Ensure connection to web socket
this.connectToDeltaStream({ reason: "autoReconnect" });
private connectInternal(args: IConnectionArgs) {
assert(!this.closed, "Attempting to connect() a closed Container");
assert(this._attachState === AttachState.Attached, "Attempting to connect() a container that is not attached");
scottn12 marked this conversation as resolved.
Show resolved Hide resolved

// Resume processing ops and connect to delta stream
this.resumeInternal(args);

// Set Auto Reconnect Mode
const mode = ReconnectMode.Enabled;
this.setAutoReconnectInternal(mode);
}

public disconnect() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add documentation.

if (this.closed) {
throw new UsageError(`The Container is closed and cannot be disconnected`);
}
else {
this.disconnectInternal();
}
}

private disconnectInternal() {
assert(!this.closed, "Attempting to disconnect() a closed Container");

// Set Auto Reconnect Mode
const mode = ReconnectMode.Disabled;
this.setAutoReconnectInternal(mode);
}

/**
* Have the container attempt to resume processing ops
* @deprecated - 0.58, This API will be removed in 1.0
* Use `connect()` instead
* See https://github.com/microsoft/FluidFramework/issues/9167 for context
*/
public resume() {
if (!this.closed) {
// Note: no need to fetch ops as we do it preemptively as part of DeltaManager.attachOpHandler().
Expand Down Expand Up @@ -1491,7 +1548,6 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i
});

deltaManager.on("disconnect", (reason: string) => {
this.manualReconnectInProgress = false;
this.collabWindowTracker.stopSequenceNumberUpdate();
this.connectionStateHandler.receivedDisconnectEvent(reason);
});
Expand Down Expand Up @@ -1558,8 +1614,6 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i
}
if (this.firstConnection) {
connectionInitiationReason = "InitialConnect";
} else if (this.manualReconnectInProgress) {
connectionInitiationReason = "ManualReconnect";
Copy link
Member

Choose a reason for hiding this comment

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

@vladsud -- Do you know if this distinction of "ManualReconnect" was leveraged much in telemetry analysis? How big a loss is this? I don't quite get the semantics of it honestly, but wanted to double-check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably? It's useful to learn how quickly document reflects latest changes after user comes back to a tab that was inactive for very long time. That was the reason for this property on connection events. I think it's interesting data to have, but with these changes I think responsibility is pushed to consumers, and I'm fine with it. But we should look closer if consumers have everything, they need to be successful here.

} else {
connectionInitiationReason = "AutoReconnect";
}
Expand All @@ -1584,7 +1638,6 @@ export class Container extends EventEmitterWithErrorHandling<IContainerEvents> i

if (value === ConnectionState.Connected) {
this.firstConnection = false;
this.manualReconnectInProgress = false;
}
}

Expand Down