From 34f83de2103f1912588300b926dbc6cc0c686d35 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Wed, 27 Feb 2019 12:12:23 -0800 Subject: [PATCH] Rollback waiting on children of a component when waiting on the component. (cherry picked from commit d1ecc5a25066e931c5ec51f75a9a33d684216809) --- CHANGELOG.md | 16 ++++- sdk/nodejs/resource.ts | 19 ------ sdk/nodejs/runtime/resource.ts | 62 ------------------- .../022.parent_child_dependencies_2/index.js | 14 +++++ sdk/nodejs/tests/runtime/langhost/run.spec.ts | 12 ++++ 5 files changed, 41 insertions(+), 82 deletions(-) create mode 100644 sdk/nodejs/tests/runtime/langhost/cases/022.parent_child_dependencies_2/index.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 96f616dfa..6c7992996 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,18 @@ -## 0.16.16 (Unreleased) +## 0.16.18 (Unreleased) + +### Improvements + +## 0.16.17 (Released February 27th, 2019) + +### Improvements + +- Rolling back the change: + "Depending on a Resource will now depend on all other Resource's parented by that Resource." + + Unforseen problems cropped up that caused deadlocks. Removing this change until we can + have a high quality solution without these issues. + +## 0.16.16 (Released February 24th, 2019) ### Improvements diff --git a/sdk/nodejs/resource.ts b/sdk/nodejs/resource.ts index 95dbdb47c..9dc3f9442 100644 --- a/sdk/nodejs/resource.ts +++ b/sdk/nodejs/resource.ts @@ -35,21 +35,6 @@ export abstract class Resource { */ public readonly urn: Output; - - /** - * The optional parent of this resource. - */ - // tslint:disable-next-line:variable-name - /* @internal */ public readonly __parentResource: Resource | undefined; - - /** - * The child resources of this resource. Used so that if any resource wants to wait on this - * resource being complete, they will logically wait on all our child resources being complete - * as well. - */ - // tslint:disable-next-line:variable-name - /* @internal */ public __childResources: Set | undefined; - /** * When set to true, protect ensures this resource cannot be deleted. */ @@ -104,10 +89,6 @@ export abstract class Resource { throw new RunError(`Resource parent is not a valid Resource: ${opts.parent}`); } - this.__parentResource = opts.parent; - this.__parentResource.__childResources = this.__parentResource.__childResources || new Set(); - this.__parentResource.__childResources.add(this); - if (opts.protect === undefined) { opts.protect = opts.parent.__protect; } diff --git a/sdk/nodejs/runtime/resource.ts b/sdk/nodejs/runtime/resource.ts index c229d98c1..49cb2cb38 100644 --- a/sdk/nodejs/runtime/resource.ts +++ b/sdk/nodejs/runtime/resource.ts @@ -269,12 +269,6 @@ async function prepareResource(label: string, res: Resource, custom: boolean, propertyToDirectDependencyURNs.set(propertyName, urns); } - // Now await all dependencies transitively. i.e. if one resource depends on some other - // component resource, then it will end up depending on all the children of that component - // resource as well. That way, a parent acts as a "logical" collection of all those children - // and will not be considered complete until all of the children are complete as well. - await getResourceURNs(getTransitivelyReferencedChildResources(allDirectDependencies, res)); - return { resolveURN: resolveURN!, resolveID: resolveID, @@ -302,62 +296,6 @@ async function getResourceURNs(resources: Set) { return result; } -/** - * Recursively walk the resources passed in, returning them and all resources reachable from - * [Resource.__childResources] - */ -function getTransitivelyReferencedChildResources(resources: Set, initialResource: Resource) { - // First, just recursively walk the dependent resources through their children, adding them to - // the result set. - const temp = new Set(); - addTransitivelyDependentResources(resources, temp); - - // Then, remove any dependent resources we found that are actually children of this resource. - // Attempting to wait on them will simply cause a deadlock. This can happen in fairly trivial - // circumstances where a child uses data from a parent. For example: - // - // const bucket = new ... - // const notification = new BucketNotification(name, { - // bucketId: bucket.id - // }, { parent: bucket }); - // - // Because notification has 'bucket' as it's parent, it will be in the child list of 'bucket'. - // However, it also depends on 'bucket' through the 'bucketId' property. We don't want to then - // have notification dependent on itself when it looks at the children of 'bucket'. - const final = new Set(); - for (const resource of temp) { - if (!isAncestor(resource, initialResource)) { - final.add(resource); - } - } - - return final; -} - -function addTransitivelyDependentResources(resources: Set | undefined, result: Set) { - if (resources) { - for (const resource of resources) { - if (!result.has(resource)) { - result.add(resource); - - addTransitivelyDependentResources(resource.__childResources, result); - } - } - } -} - -function isAncestor(resource: Resource | undefined, potentialAncestor: Resource) { - while (resource) { - if (resource === potentialAncestor) { - return true; - } - - resource = resource.__parentResource; - } - - return false; -} - /** * Gathers explicit dependent Resources from a list of Resources (possibly Promises and/or Outputs). */ diff --git a/sdk/nodejs/tests/runtime/langhost/cases/022.parent_child_dependencies_2/index.js b/sdk/nodejs/tests/runtime/langhost/cases/022.parent_child_dependencies_2/index.js new file mode 100644 index 000000000..49e0229c2 --- /dev/null +++ b/sdk/nodejs/tests/runtime/langhost/cases/022.parent_child_dependencies_2/index.js @@ -0,0 +1,14 @@ +// Test the ability to invoke provider functions via RPC. + +let assert = require("assert"); +let pulumi = require("../../../../../"); + +class MyResource extends pulumi.CustomResource { + constructor(name, args, opts) { + super("test:index:MyResource", name, args, opts); + } +} + +let resA = new MyResource("resA"); +let resB = new MyResource("resB", { parentId: resA.id }, { parent: resA }); +let resC = new MyResource("resC", { parentId: resA.id }, { parent: resA }); diff --git a/sdk/nodejs/tests/runtime/langhost/run.spec.ts b/sdk/nodejs/tests/runtime/langhost/run.spec.ts index f059473dd..fa5a70464 100644 --- a/sdk/nodejs/tests/runtime/langhost/run.spec.ts +++ b/sdk/nodejs/tests/runtime/langhost/run.spec.ts @@ -604,9 +604,21 @@ describe("rpc", () => { return { urn: makeUrn(t, name), id: undefined, props: undefined }; }, }, + "parent_child_dependencies_2": { + pwd: path.join(base, "022.parent_child_dependencies_2"), + program: "./index.js", + expectResourceCount: 3, + registerResource: (ctx: any, dryrun: boolean, t: string, name: string) => { + return { urn: makeUrn(t, name), id: undefined, props: undefined }; + }, + }, }; for (const casename of Object.keys(cases)) { + // if (casename !== "parent_child_dependencies_2") { + // continue; + // } + const opts: RunCase = cases[casename]; it(`run test: ${casename} (pwd=${opts.pwd},prog=${opts.program})`, asyncTest(async () => { // For each test case, run it twice: first to preview and then to update.