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

Plan for fully removing dependency on deasync and entirely removing this avenue for hangs/crashes. #3528

Closed
4 of 14 tasks
CyrusNajmabadi opened this issue Nov 19, 2019 · 6 comments
Assignees
Labels
p1 A bug severe enough to be the next item assigned to an engineer
Milestone

Comments

@CyrusNajmabadi
Copy link
Contributor

CyrusNajmabadi commented Nov 19, 2019

Here's the current plan - original details below for historical context.

Phase 1 (Q4'19):

  • Remove use of synchronous invokes in awsx (potentially a breaking change for some AWSX APIs)
  • Remove use of synchronous invokes in eks
  • Remove use of synchronous invokes in all examples
  • Remove use of synchronous invokes in tf2pulumi generated code (including all docs)

Phase 2 (H1'20):


Note: requires breaking changes in Pulumi APIs and will require updates to user programs.

First, the list of work necessary is as follows:

Justifications for the above work:

Currently, it is still possible for users to run into hangs/crashes due to deasync doing normal development using Pulumi. This can happen when a user uses a ProviderResource (like aws.Provider) and either:

  1. directly calls into a synchronous data-source
  2. uses a library like awsx which then indirectly calls a synchronous data-source for them.

This issue is one that users can work around, but not in a pleasant manner. First, the normal development model we recommend can lead to them hitting this instead of avoiding it. Second, the workaround for the issue (calling await ProviderResource.register) is both undiscoverable and often unpleasant to contort the code into. This workaround is definitely necessary to have to give people a way to cheaply deal with the issue, but is not a healthy place for our platform and sdks for the medium and long term.

A longer term solution is necessary where this problem no longer exists. Fundamentally, due to the need to actually support async providers (very important for k8s scenarios), our hands are forced, and we must bubble up asynchrony through our platform and then through components built on top of that. To that end, a full solution to this issue looks like the following:

  1. Entirely remove deasync from the @pulumi/pulumi SDK. This means
    1.1. No more synchronous data-source 'invoke' (very impactful). (async invokes will exist).
    1.2. No more 'StackReference-getOutputSync' on a stack reference. (there will be a way to get the values asynchronously).
  2. Removing synchronous data-sources means that all data-source will be promise-returning and will impact all resource sdks.
  3. Many/Most/All APIs in our component libraries will need to become async.
    3.1 Even if a component api does not need any data-sources, it may end up needing that in the future, warranting a breaking change if we don't start with it being async.
    3.2 This likely means constructors for component resources are not directly exposed. Instead, async
    factory methods will be needed to ensre that components can asynchronously get the information they need for async-data-sources and then perform their necessary flow-control off of them.

Fundamentally, this is just pushing an async model through data-sources/components. Because of the viral nature of async, it is likely this will push the user to need to be async throughout their program. While JS/Node are inching toward a future where that is fully supported and pleasant, they will likely not get there for some time. It will also only be pleasant for those on the bleeding edge of Node. Because of this, we should likely also include #3321 in our work. This PR makes it possible for a user to export a top level async function as the start of their Pulumi program. i.e.:

// main.ts
import * as pulumi from "@pulumi/pulumi";
import * as awsx from "@pulumi/awsx";

export = async () => {
   const vpc = await awsx.ec2.Vpc.create(...);
   const cluster = awsx.ecs.Cluster.create(...);

   return { vpcId: vpc.id, ... };
}

While not as attractively simple as just a plain synchronous JS/TS app, this is very low friction for the user. Specifically, practically any existing Pulumi program can transition to this with mostly mechanical effort.

This entire effort will fundamentally break/deprecate existing code that performs synchronous data-source calls or uses our synchronous components. It will need to come with a major semver update. Along with that, we will need to update our docs/examples accordingly.

@CyrusNajmabadi
Copy link
Contributor Author

Tagging @lukehoban @pgavlin

@CyrusNajmabadi CyrusNajmabadi self-assigned this Nov 19, 2019
@pgavlin
Copy link
Member

pgavlin commented Nov 20, 2019

No more 'StackReference-getOutputSync' on an async stack reference. This is less impactful.

Can you elaborate on this?

@CyrusNajmabadi
Copy link
Contributor Author

CyrusNajmabadi commented Nov 20, 2019

Can you elaborate on this?

yes:

if (this.stackReferenceName instanceof Promise) {
// Have to do an explicit console.log here as the call to utils.promiseResult may hang
// node, and that may prevent our normal logging calls from making it back to the user.
console.log(
`Call made to StackReference.${callerName} with a StackReference with a Promise name. This is now deprecated and may cause the program to hang.
For more details see: https://www.pulumi.com/docs/troubleshooting/#stackreference-sync`);
stackName = promiseResult(this.stackReferenceName);
}
else if (Output.isInstance(this.stackReferenceName)) {
console.log(
`Call made to StackReference.${callerName} with a StackReference with an Output name. This is now deprecated and may cause the program to hang.
For more details see: https://www.pulumi.com/docs/troubleshooting/#stackreference-sync`);
stackName = promiseResult(this.stackReferenceName.promise());
}

if the stack-reference name is itself an async-input, then we are forced to use deasync (the calls to promiseResult in the above code). I hope we can simply deprecate this functionality and require that the name of a StackReference must be non-async.

Does that help? Do you see any serious concerns around deprecatin the ability to pass in async-names to StackReference?

@pgavlin
Copy link
Member

pgavlin commented Nov 21, 2019

Does that help? Do you see any serious concerns around deprecatin the ability to pass in async-names to StackReference?

The thing that worries me is the inconsistency. I would have imagined that rather than deprecate the ability to pass in the name as an Input<string>, we would deprecate getOuptutSync et. al. and instead provide async replacements.

@CyrusNajmabadi
Copy link
Contributor Author

The thing that worries me is the inconsistency. I would have imagined that rather than deprecate the ability to pass in the name as an Input, we would deprecate getOuptutSync et. al. and instead provide async replacements.

Yes, that sounds reasonable. I will update the plan and i can start this on monday.

@pgavlin pgavlin added this to the 0.30 milestone Dec 3, 2019
@pgavlin pgavlin added the p1 A bug severe enough to be the next item assigned to an engineer label Dec 3, 2019
lukehoban added a commit to pulumi/pulumi-eks that referenced this issue Dec 17, 2019
Also allow passsing a parent to inherit provider from.

Part of pulumi/pulumi#3528.
lukehoban added a commit to pulumi/pulumi-eks that referenced this issue Dec 17, 2019
Also allow passsing a parent to inherit provider from.

Part of pulumi/pulumi#3528.
lukehoban added a commit to pulumi/examples that referenced this issue Dec 17, 2019
This standardizes all data source calls to pass `async: true` to avoid potential for hangs.  See pulumi/pulumi#3528.
lukehoban added a commit to pulumi/examples that referenced this issue Dec 17, 2019
This standardizes all data source calls to pass `async: true` to avoid potential for hangs.  See pulumi/pulumi#3528.
lukehoban added a commit to pulumi/pulumi-eks that referenced this issue Dec 17, 2019
Also allow passsing a parent to inherit provider from.

Part of pulumi/pulumi#3528.
@lukehoban
Copy link
Member

We landed pulumi/pulumi-awsx#470 which was one of the biggest pieces of this work. Given progress, closing this out - and will track remaining work in the issues linked above.

dixler pushed a commit to pulumi/examples that referenced this issue Jan 21, 2022
This standardizes all data source calls to pass `async: true` to avoid potential for hangs.  See pulumi/pulumi#3528.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p1 A bug severe enough to be the next item assigned to an engineer
Projects
None yet
Development

No branches or pull requests

3 participants