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

Make resource functions blocking #331

Closed
joeduffy opened this issue Sep 7, 2017 · 9 comments
Closed

Make resource functions blocking #331

joeduffy opened this issue Sep 7, 2017 · 9 comments
Assignees
Labels
kind/enhancement Improvements or new features
Milestone

Comments

@joeduffy
Copy link
Member

joeduffy commented Sep 7, 2017

At the moment, we serialize closures asynchronously. This permits us to capture references to resources whose properties may not have settled yet. Sadly, it suffers from two problems:

  • Value captures now happen at indeterminate points in time.
  • It is possible to create cycles, which themselves create hangs.

As an example, the Pulumi Framework's crawler example contains this code:

import * as pulumi from "@pulumi/pulumi";

let countDown = new pulumi.Topic<number>("countDown");

countDown.subscribe("watcher", async (num) => {
    console.log(num);
    if (num > 0) {
        await countDown.publish(num - 1);
    }
});

pulumi.timer.interval("heartbeat", {minutes: 5}, async () => {
    await countDown.publish(25);
});

Subtly, the lambda passed to countDown.subscribe closes over countDown itself, and that object internally stores a reference to the aws.lambda.Function that results from serializing the closure. On its face, this should not be possible! The ordering strictly happens as: 1) create the topic, 2) create the function, 3) subscribe. But the use of promises allows us to fast forward in time such that the capture actually depends on the function having been created!

Yikes.

After landing the current Pulumi Fabric overhaul, I'm going to go back and overhaul the overhaul. Most likely, we will do something like this:

  1. Hide all waits and resolutions in C++ code so that we can block safely while still processing incoming RPC requests from the host resource monitor (containing property resolutions).
  2. Turn Resource into a JavaScript Proxy, so we can hook access to properties, and transparently block by calling into the C++ code. This will eliminate Computed<T>, mapValue, and vastly simplify consumer code. I dare say, it will be gorgeous.

So, good news, we see a path to something awesome. Bad news, it's more work.

@joeduffy joeduffy added area/core kind/enhancement Improvements or new features labels Sep 7, 2017
@joeduffy joeduffy added this to the 0.6 milestone Sep 7, 2017
@joeduffy joeduffy self-assigned this Sep 7, 2017
joeduffy added a commit to pulumi/pulumi-cloud that referenced this issue Sep 7, 2017
This causes deadlocks for reasons explained in pulumi/pulumi#331.
As soon as those problems are taken care of, we will reenable this.
joeduffy added a commit to pulumi/pulumi-cloud that referenced this issue Sep 9, 2017
The Swagger spec object can contain embedded computed objects, so
we actually can't compute its value entirely promptly anymore.  (Previously
we JSON serialized it using our custom runtime implementation, which
apparently tolerated unavailable computed values.)  This change properly
mapValue-ifies the serialization logic, in all its ugly glory.  Unfortunately,
this also means we can't promptly compute the hash based entirely on the
values, which is used for resource object ID calculation.  For now, we will
just skip the non-prompt parts; this will lead to false hash collisions, but
anything else would be super hacky and all of this will get better imminently
once the current round of changes land and we can tackle pulumi/pulumi#331.
joeduffy added a commit to pulumi/pulumi-cloud that referenced this issue Sep 9, 2017
The Swagger spec object can contain embedded computed objects, so
we actually can't compute its value entirely promptly anymore.  (Previously
we JSON serialized it using our custom runtime implementation, which
apparently tolerated unavailable computed values.)  This change properly
mapValue-ifies the serialization logic, in all its ugly glory.  Unfortunately,
this also means we can't promptly compute the hash based entirely on the
values, which is used for resource object ID calculation.  For now, we will
just skip the non-prompt parts; this will lead to false hash collisions, but
anything else would be super hacky and all of this will get better imminently
once the current round of changes land and we can tackle pulumi/pulumi#331.
joeduffy added a commit to pulumi/pulumi-cloud that referenced this issue Sep 9, 2017
The Swagger spec object can contain embedded computed objects, so
we actually can't compute its value entirely promptly anymore.  (Previously
we JSON serialized it using our custom runtime implementation, which
apparently tolerated unavailable computed values.)  This change properly
mapValue-ifies the serialization logic, in all its ugly glory.  Unfortunately,
this also means we can't promptly compute the hash based entirely on the
values, which is used for resource object ID calculation.  For now, we will
just skip the non-prompt parts; this will lead to false hash collisions, but
anything else would be super hacky and all of this will get better imminently
once the current round of changes land and we can tackle pulumi/pulumi#331.
@joeduffy
Copy link
Member Author

Bad news: I don't think we can hide Computed<T> and mapValue just yet.

The two ideas we had for doing so entailed first moving blocking into C++ code (which we will do), and then one of the following:

  1. Expose a get method that simply returns the T.
  2. Atop this same machinery, use JavaScript proxies to hide the get.

Sadly, these both suffer from the fact that we cannot produce a real T for such properties during planning: we simply don't know the values, anything we create would be a poor approximation of the real thing, and it would be dangerous to let code do conditional resource creation using them.

I'm not loving where this lands us. The Computed<T> and whenValue stuff makes me sad anytime I need to use it. I'll keep pondering...

@lukehoban
Copy link
Member

What happens if we just return undefined for properties on Resources during planning? It doesn’t feel meaningfully worse than returning a Computed which never resolves.

@joeduffy
Copy link
Member Author

Certainly an option we should keep in the running. I don't love that it would result in a lot of conditional code, but a) experience suggests that this sort of thing is fairly rare, and b) mapValue is conditional anyway, under a different name (and hence could lead to lots of the same pitfalls).

@lukehoban
Copy link
Member

mapValue is conditional anyway, under a different name (and hence could lead to lots of the same pitfalls)

Indeed - I noticed in your refactors of code in pulumi-framework that you had to go to some effort to make sure that resource creations didn't happen within the body of a mapValue. In some sense, because the programming model makes it look like you can get an actual value that way, it's easier to try to write the "conditional" code that way accidentally, vs. writing code which is conditional on an undefined result only during planning (you can't try to do much interesting in that branch cause you can't see anything about the value that got you into the branch).

@joeduffy joeduffy modified the milestones: 0.6, 0.7 Sep 15, 2017
joeduffy added a commit that referenced this issue Sep 20, 2017
As part of #331, we've been exploring just using
undefined to indicate that a property value is absent during planning.
We also considered blocking the message loop to simplify the overall
programming model, so that all asynchrony is hidden.

It turns out ThereBeDragons 🐲 anytime you try to block the
message loop.  So, we aren't quite sure about that bit.

But the part we are convicted about is that this Computed/Property
model is far too complex.  Furthermore, it's very close to promises, and
yet frustratingly so far away.  Indeed, the original thinking in
#271 was simply to use promises, but we wanted to
encourage dataflow styles, rather than control flow.  But we muddied up
our thinking by worrying about awaiting a promise that would never resolve.

It turns out we can achieve a middle ground: resolve planning promises to
undefined, so that they don't lead to hangs, but still use promises so
that asynchrony is explicit in the system.  This also avoids blocking the
message loop.  Who knows, this may actually be a fine final destination.
joeduffy added a commit that referenced this issue Sep 20, 2017
As part of #331, we've been exploring just using
undefined to indicate that a property value is absent during planning.
We also considered blocking the message loop to simplify the overall
programming model, so that all asynchrony is hidden.

It turns out ThereBeDragons 🐲 anytime you try to block the
message loop.  So, we aren't quite sure about that bit.

But the part we are convicted about is that this Computed/Property
model is far too complex.  Furthermore, it's very close to promises, and
yet frustratingly so far away.  Indeed, the original thinking in
#271 was simply to use promises, but we wanted to
encourage dataflow styles, rather than control flow.  But we muddied up
our thinking by worrying about awaiting a promise that would never resolve.

It turns out we can achieve a middle ground: resolve planning promises to
undefined, so that they don't lead to hangs, but still use promises so
that asynchrony is explicit in the system.  This also avoids blocking the
message loop.  Who knows, this may actually be a fine final destination.
joeduffy added a commit that referenced this issue Sep 20, 2017
As part of #331, we've been exploring just using
undefined to indicate that a property value is absent during planning.
We also considered blocking the message loop to simplify the overall
programming model, so that all asynchrony is hidden.

It turns out ThereBeDragons 🐲 anytime you try to block the
message loop.  So, we aren't quite sure about that bit.

But the part we are convicted about is that this Computed/Property
model is far too complex.  Furthermore, it's very close to promises, and
yet frustratingly so far away.  Indeed, the original thinking in
#271 was simply to use promises, but we wanted to
encourage dataflow styles, rather than control flow.  But we muddied up
our thinking by worrying about awaiting a promise that would never resolve.

It turns out we can achieve a middle ground: resolve planning promises to
undefined, so that they don't lead to hangs, but still use promises so
that asynchrony is explicit in the system.  This also avoids blocking the
message loop.  Who knows, this may actually be a fine final destination.
joeduffy added a commit that referenced this issue Sep 20, 2017
As part of #331, we've been exploring just using
undefined to indicate that a property value is absent during planning.
We also considered blocking the message loop to simplify the overall
programming model, so that all asynchrony is hidden.

It turns out ThereBeDragons 🐲 anytime you try to block the
message loop.  So, we aren't quite sure about that bit.

But the part we are convicted about is that this Computed/Property
model is far too complex.  Furthermore, it's very close to promises, and
yet frustratingly so far away.  Indeed, the original thinking in
#271 was simply to use promises, but we wanted to
encourage dataflow styles, rather than control flow.  But we muddied up
our thinking by worrying about awaiting a promise that would never resolve.

It turns out we can achieve a middle ground: resolve planning promises to
undefined, so that they don't lead to hangs, but still use promises so
that asynchrony is explicit in the system.  This also avoids blocking the
message loop.  Who knows, this may actually be a fine final destination.
@lukehoban
Copy link
Member

I hit another nasty implication of the current model - the types of a captured variable is different on the outside than on the inside.

class Table {
    tableName: pulumi.Computed<string>;
    constructor() {
        let table = new Table();
        this.tableName = table.name;
    }
    async get(query: Object): Promise<any> {
        let awssdk = require("aws-sdk");
        let db = new awssdk.DynamoDB.DocumentClient();
        let o = await db.get({
            TableName: this.tableName,
            Key: query,
        }).promise();
        return o.Item;
    }
}

Here - this.tableName ends up being a Computed<string> on the outside, but a string on the inside.

We only get away with this in our current code because we don't have strong typing around most of the code we write on the inside today - due to the previous requirement to require on the inside instead of use real imports to bring in types.

This makes explaining our outside/inside API another big step harder. I continue to be optimistic we'll find a way to remove this layer of Computed types in the programming model.

@joeduffy joeduffy changed the title Asynchronous closure serialization is problematic Make resource property access blocking Oct 8, 2017
@joeduffy joeduffy modified the milestones: 0.7, 0.8 Oct 8, 2017
joeduffy added a commit that referenced this issue Oct 11, 2017
This is a partial implementation of blocking properties, as outlined in
#331.  The idea here is to use a magical combination of many
advanced (and in some cases, evil) features to give the developers a simpler
programming model that is promises-free overall.  For example, rather than

    let res = new MyResource(...);
    let id: Promise<Computed<ID>> = res.id;
    res.id.then((id: Computed<ID>) => {
        // use id
    });

you can simply say

    let res = new MyResource(...);
    let id: Computed<ID> = res.id;
    // use id

Of course, due to all the RPC going on under the hood, there are still
promises and so on, but we hide them in the following ways:

    * All property values on Resources are actually runtime.Property
      objects at runtime.  These internally contain nativeruntime.BlockingPromises.

    * nativeruntime.BlockingPromise is a promise that uses C++11 futures
      to actually block and resolve values on the native code side of things.

    * As a result, we can call await() on them to block the Node.js message
      pump entirely, and then just get back a raw T, the underlying property value.

    * All Resources are wrapped in an ES6 Proxy inside of the base constructor
      so that all access to properties can be hooked, and so we can silently
      inject calls to await() on properties that are of the runtime type Property.

This is all good and well, but we're blocking the message pump.  In most cases,
this would be a disastrously terrible idea.  However, in our specific case, because
we have no cycles between Resources, and because we carefully control the
creation and resolution of all Property objects, we can delicately ensure that
deadlocks do not occur.  Well, that's the theory at least ... 😬

As written, however, it is chock full of deadlocks.  That is the second part of
the change that will come next.  Next, we must rewrite our RPC handlers in C++
so that the processing of RPC calls, and responses to them, do not depend on
running the Node.js message pump (which leads to circular dependencies).
@joeduffy
Copy link
Member Author

I think we have an approach to try here. I'll write up and give it another run during M9.

@joeduffy
Copy link
Member Author

joeduffy commented Dec 1, 2017

Sadly, this is too disruptive to take at this stage. Moving to M10.

@joeduffy joeduffy modified the milestones: 0.9, 0.10 Dec 1, 2017
joeduffy added a commit that referenced this issue Dec 28, 2017
This change adds a basic Python langhost RPC server.  It's fairly
barebones and merely acts as a jumping off point for the Pulumi engine
to spawn a Python program.  The host is written in Go, in contrast to
implementing the host in Python, and more closely resembles how I
expect the Node.js language host to work once #331 is done.
@joeduffy joeduffy assigned CyrusNajmabadi and unassigned joeduffy Jan 11, 2018
joeduffy added a commit that referenced this issue Jan 13, 2018
This change adds a basic Python langhost RPC server.  It's fairly
barebones and merely acts as a jumping off point for the Pulumi engine
to spawn a Python program.  The host is written in Go, in contrast to
implementing the host in Python, and more closely resembles how I
expect the Node.js language host to work once #331 is done.
@joeduffy joeduffy changed the title Make resource property access blocking Make resource functions blocking Feb 1, 2018
@joeduffy
Copy link
Member Author

joeduffy commented Feb 1, 2018

I've changed the title, since as part of #824 we decided against introducing blocking resource property access. But we did decide that resource functions should offer blocking variants as the default. Moving to M11 for consideration.

@joeduffy joeduffy modified the milestones: 0.10, 0.11 Feb 1, 2018
@joeduffy joeduffy modified the milestones: 0.11, 0.12 Feb 12, 2018
joeduffy added a commit that referenced this issue Feb 21, 2018
This change adds a basic Python langhost RPC server.  It's fairly
barebones and merely acts as a jumping off point for the Pulumi engine
to spawn a Python program.  The host is written in Go, in contrast to
implementing the host in Python, and more closely resembles how I
expect the Node.js language host to work once #331 is done.
joeduffy added a commit that referenced this issue Feb 24, 2018
This change adds a basic Python langhost RPC server.  It's fairly
barebones and merely acts as a jumping off point for the Pulumi engine
to spawn a Python program.  The host is written in Go, in contrast to
implementing the host in Python, and more closely resembles how I
expect the Node.js language host to work once #331 is done.
@lukehoban lukehoban modified the milestones: 0.12, 0.16 Mar 11, 2018
@lukehoban
Copy link
Member

We've decided not to do this. Current async variants compose well into Output, especially once we address pulumi/pulumi-terraform#204.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement Improvements or new features
Projects
None yet
Development

No branches or pull requests

3 participants