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 a supported api for components to indicate that they are asynchronously constructed. #3676

Merged
merged 20 commits into from
Dec 17, 2019

Conversation

CyrusNajmabadi
Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Dec 17, 2019

ComponentResource now has a virtual initialize method that subclasses can use to place async construction work inside of. For example, if the component needs to asynchronously call into data-sources (i.e. aws.getAvailabilityZones).

The general pattern one should use is:

class MyComponent extends ComponentResource<MyComponentData> {
  public readonly prop: Output<Type>;
  public readonly child: Output<ChildResource>;

  constructor(name: string, args: MyComponentArgs, opts: ComponentResourceOptions) {
    super("...", name, {}, opts);
    const data: Promise<MyComponentData> = this.getData();
    this.prop1 = pulumi.output(data.then(d => d.prop));
    this.child = pulumi.output(data.then(d => d.child));
  }

  /** @override */
  async initialize(): Promise<MyComponentData> {
    // do async work
    const child = new ChildResource(..., { parent: this});
    return { prop: ..., child: child };
  }
}

Note that the new method must be called initialize.

Pulumi understands this pattern and will wait until MyComponent is entirely finished initializing when it is used in other locations. For example, if some otehr component "depends on" a MyComponent instance, it will only proceed once MyComponent and it's child: Resource are finished construction.

Prior to this, Pulumi would only wait until MyComponent's constructor completed, making it difficult to depend on this sort of async work finishing.

@@ -66,7 +67,7 @@ class Stack extends ComponentResource {
*
* @param init The callback to run in the context of this Pulumi stack
*/
private async runInit(init: () => Promise<Inputs>): Promise<Inputs | undefined> {
async initialize(args: { init: () => Promise<Inputs> }): Promise<Inputs> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

stacks were always async constructed before. Now, we can just use the built-in system for that.

@CyrusNajmabadi CyrusNajmabadi merged commit 342b80b into master Dec 17, 2019
@pulumi-bot pulumi-bot deleted the asyncComponents2 branch December 17, 2019 23:34

private async initializeAndRegisterOutputs(args: Inputs) {
const data = await this.initialize(args);
this.registerOutputs();
Copy link
Member

Choose a reason for hiding this comment

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

@CyrusNajmabadi @pgavlin This seems like it leads to all existing ComponentResources calling registerOutputs immediately during the super call if they don't override initialize. That's a significant change of behaviour, doesn't sound "right" (even in the normal case, this should only happen at the end of synchronous construction), feels like it leads to even more unhelpful status reporting on components (I think they will all claim they are done immediately now?), and I can't tell why it doesn't break all existing ComponentResources which themselves already call .registerOutputs at the end of construction (since it will already have been called).

I'm fairly certain this is wrong and we need to revert - but I'm not entirely clear on what the design goal was here?

Copy link
Member

Choose a reason for hiding this comment

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

I can't tell why it doesn't break all existing ComponentResources which themselves already call .registerOutputs at the end of construction (since it will already have been called).

I see now there is new code here to specifically avoid that leading to an error - but that doesn't feel right either - we don't really want to allow multiple calls to registerOutputs - we really do want anyone trying to call this a second time to fail as they are not getting the effect they want (in this case, the effect that every existing component is trying to get but is no longer getting I believe).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we really do want anyone trying to call this a second time to fail as they are not getting the effect they want (in this case, the effect that every existing component is trying to get but is no longer getting I believe).

So. The thinking that went into this was the following:

  1. for components there is no real usecase (outside of Stacks) for components to register actual values in registerOutputs. Indeed, a long time ago, we went to all our components and changed them to just call .registerOutputs() (i.e. with no args).
  2. registerOutputs then simply became the way for a componetn to tell the UI i'm done initializing. This was purely for the UI so we could change the status from "running..." to "complete".

Because of this, and with the introduction of async components it became unnecessary for a component to call .registerOutputs. We will call it for them at a time (thanks to JS semantics) we know is after the derived constructor finishes.

Either:

  1. the component is synchronous. In which case, we still call .registerOutputs for them after their constructor finished.
  2. the component is async. In which case (if they override async initialize()) we will also run .registerOutputs after them.

The only time we run before htem is if they finish constructing after their constructor, and don't use initialize(). And even in that case, all that happens is hte UI says they completed earlier than normal.

This does not seem like a very bad thing, and i think the wins in the programming model here are worth it for the change in that corner case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants