From dfcb17d3682a0a1c9aede15556fa3194a664feee Mon Sep 17 00:00:00 2001 From: Tony Murphy Date: Wed, 16 Dec 2020 15:42:27 -0800 Subject: [PATCH] Throw when loading fails in SharedString (#4615) fixes #4612 releated #4613 --- packages/dds/merge-tree/src/snapshotLoader.ts | 3 + packages/dds/sequence/src/sequence.ts | 6 +- .../test/sharedStringEndToEndTests.spec.ts | 3 +- .../src/test/sharedStringLoading.spec.ts | 132 ++++++++++++++++++ 4 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 packages/test/end-to-end-tests/src/test/sharedStringLoading.spec.ts diff --git a/packages/dds/merge-tree/src/snapshotLoader.ts b/packages/dds/merge-tree/src/snapshotLoader.ts index d13f5d764aeb..f56a067dc2f7 100644 --- a/packages/dds/merge-tree/src/snapshotLoader.ts +++ b/packages/dds/merge-tree/src/snapshotLoader.ts @@ -46,6 +46,9 @@ export class SnapshotLoader { const catchupOpsP = this.loadBodyAndCatchupOps(headerLoadedP, services); + catchupOpsP.catch( + (err)=>this.logger.sendErrorEvent({ eventName: "CatchupOpsLoadFailure" },err)); + await headerLoadedP; return { catchupOpsP }; diff --git a/packages/dds/sequence/src/sequence.ts b/packages/dds/sequence/src/sequence.ts index f8259bbb404c..e16ad5e859ee 100644 --- a/packages/dds/sequence/src/sequence.ts +++ b/packages/dds/sequence/src/sequence.ts @@ -124,6 +124,10 @@ export abstract class SharedSegmentSequence ) { super(id, dataStoreRuntime, attributes); + this.loadedDeferred.promise.catch((error)=>{ + this.logger.sendErrorEvent({ eventName: "SequenceLoadFailed" }, error); + }); + this.client = new MergeTree.Client( segmentFromSpec, ChildLogger.create(this.logger, "SharedSegmentSequence.MergeTreeClient"), @@ -654,8 +658,8 @@ export abstract class SharedSegmentSequence // Initialize the interval collections this.initializeIntervalCollections(); if (error) { - this.logger.sendErrorEvent({ eventName: "SequenceLoadFailed" }, error); this.loadedDeferred.reject(error); + throw error; } else { // it is important this series remains synchronous // first we stop defering incoming ops, and apply then all diff --git a/packages/test/end-to-end-tests/src/test/sharedStringEndToEndTests.spec.ts b/packages/test/end-to-end-tests/src/test/sharedStringEndToEndTests.spec.ts index ac612dbcaa13..8b81b6ddf812 100644 --- a/packages/test/end-to-end-tests/src/test/sharedStringEndToEndTests.spec.ts +++ b/packages/test/end-to-end-tests/src/test/sharedStringEndToEndTests.spec.ts @@ -62,7 +62,8 @@ const tests = (args: ITestObjectProvider) => { const newContainer = await args.loadTestContainer(testContainerConfig) as Container; const newComponent = await requestFluidObject(newContainer, "default"); const newSharedString = await newComponent.getSharedObject(stringId); - assert.equal(newSharedString.getText(), text, "The new container should receive the inserted text on creation"); + assert.equal( + newSharedString.getText(), text, "The new container should receive the inserted text on creation"); }); }; diff --git a/packages/test/end-to-end-tests/src/test/sharedStringLoading.spec.ts b/packages/test/end-to-end-tests/src/test/sharedStringLoading.spec.ts new file mode 100644 index 000000000000..f662ff5dfa97 --- /dev/null +++ b/packages/test/end-to-end-tests/src/test/sharedStringLoading.spec.ts @@ -0,0 +1,132 @@ +/*! + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. + */ + +import { strict as assert } from "assert"; +import { Loader } from "@fluidframework/container-loader"; +import { requestFluidObject } from "@fluidframework/runtime-utils"; +import { SharedString } from "@fluidframework/sequence"; +import { + ChannelFactoryRegistry, + ITestFluidObject, + LocalCodeLoader, + SupportedExportInterfaces, + TestFluidObjectFactory, +} from "@fluidframework/test-utils"; +import { LocalDeltaConnectionServer } from "@fluidframework/server-local-server"; +import { LocalDocumentServiceFactory, LocalResolver } from "@fluidframework/local-driver"; +import { + IDocumentService, + IDocumentServiceFactory, + IDocumentStorageService, + LoaderCachingPolicy, + } from "@fluidframework/driver-definitions"; +import { NetworkErrorBasic } from "@fluidframework/driver-utils"; +import { IFluidHandle } from "@fluidframework/core-interfaces"; + +describe("SharedString", () => { + it("Failure to Load in Shared String", async ()=>{ + const deltaConnectionServer = LocalDeltaConnectionServer.create(); + const stringId = "sharedStringKey"; + const registry: ChannelFactoryRegistry = [[stringId, SharedString.getFactory()]]; + const fluidExport: SupportedExportInterfaces = { + IFluidDataStoreFactory: new TestFluidObjectFactory(registry), + }; + const text = "hello world"; + const documentId = "sstest"; + { // creating client + const urlResolver = new LocalResolver(); + const documentServiceFactory = new LocalDocumentServiceFactory(deltaConnectionServer); + const codeDetails = { package: "no-dynamic-pkg" }; + const codeLoader = new LocalCodeLoader([ + [codeDetails, fluidExport], + ]); + + const loader = new Loader({ + urlResolver, + documentServiceFactory, + codeLoader, + }); + + const container = await loader.createDetachedContainer(codeDetails); + const dataObject = await requestFluidObject(container, "default"); + const sharedString = await dataObject.root.get>(stringId).get(); + sharedString.insertText(0, text); + + await container.attach(urlResolver.createCreateNewRequest(documentId)); + } + { // normal load client + const urlResolver = new LocalResolver(); + const documentServiceFactory = new LocalDocumentServiceFactory(deltaConnectionServer); + const codeDetails = { package: "no-dynamic-pkg" }; + const codeLoader = new LocalCodeLoader([ + [codeDetails, fluidExport], + ]); + + const loader = new Loader({ + urlResolver, + documentServiceFactory, + codeLoader, + }); + + const container = await loader.resolve(urlResolver.createCreateNewRequest(documentId)); + const dataObject = await requestFluidObject(container, "default"); + const sharedString = await dataObject.root.get>(stringId).get(); + assert.strictEqual(sharedString.getText(0), text); + } + { // failure load client + const urlResolver = new LocalResolver(); + const realSf: IDocumentServiceFactory = + new LocalDocumentServiceFactory(deltaConnectionServer); + const documentServiceFactory: IDocumentServiceFactory = { + ...realSf, + createDocumentService: async (resolvedUrl,logger) => { + const realDs = await realSf.createDocumentService(resolvedUrl, logger); + const mockDs = Object.create(realDs) as IDocumentService; + mockDs.policies = { + ... mockDs.policies, + caching: LoaderCachingPolicy.NoCaching, + }; + mockDs.connectToStorage = async ()=>{ + const realStorage = await realDs.connectToStorage(); + const mockstorage = Object.create(realStorage) as IDocumentStorageService; + mockstorage.read = async (id)=>{ + const blob = await realStorage.read(id); + const blobObj = JSON.parse(Buffer.from(blob, "Base64").toString()); + // throw when trying to load the header blob + if (blobObj.headerMetadata !== undefined) { + throw new NetworkErrorBasic( + "Not Found", + undefined, + false, + 404); + } + return blob; + }; + return mockstorage; + }; + return mockDs; + }, + }; + const codeDetails = { package: "no-dynamic-pkg" }; + const codeLoader = new LocalCodeLoader([ + [codeDetails, fluidExport], + ]); + + const loader = new Loader({ + urlResolver, + documentServiceFactory, + codeLoader, + }); + + const container = await loader.resolve(urlResolver.createCreateNewRequest(documentId)); + const dataObject = await requestFluidObject(container, "default"); + // TODO: this should probably throw, but currently returns the dataObject due to #4613 + // this test should be updated when that issue is fix + const sharedString = await dataObject.root.get>(stringId).get(); + // eslint-disable-next-line dot-notation + assert.strictEqual(sharedString["getText"], undefined); + } + }); +});