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

stackeval: Improve coverage of namedPromiseReporter impls #35153

Merged
merged 2 commits into from
May 16, 2024

Conversation

apparentlymart
Copy link
Member

@apparentlymart apparentlymart commented May 13, 2024

The namedPromiseReporter interface is a helper we use to collect user-friendly names for promises when we need to report a promise self-reference error to the user.

We collect names for promises reactively on error, rather than proactively in the normal course of work, because proactively maintaining a suitable data structure for this cross-cutting concern would be both expensive and complicated.

This compromise therefore allows us to pay the cost of walking over all of the promise-owning objects only in the exceptional case where we need to report a self-reference error, but it does come at the expense of it being possible for us to forget to implement this interface for certain types and thus be unable to name a subset of the affected promises.

(Those who are familiar with the implementation details of Terraform's modules runtime might think of this as being roughly analogous to calling dag.VertexName on all of the graph nodes in a dependency cycle, but in this case the detection of the cycle is done by package promising while the reporting is done by package stackeval, so the parts take on a different shape.)


This commit implements reportNamedPromises for all promise-owning types that are reachable from a stackeval.Main, although there is a notable gap here where we can't actually report promises from the individual instances of a multi-instance object (e.g. a component) because the promising API doesn't let us peek into a promising.Once to see if it's resolved without blocking until it becomes resolved.

There are FIXME comments for that limitation in all of the places where it's relevant, and so hopefully in a future commit we'll devise a suitable way to perform a fallible non-blocking peek of a promise so that we can report promises nested in its result only if they are already present.

There were already implementations in place for the "Config" variants of these types from the original work on implementing the validation phase, so this is just plugging gaps we missed when later implementing plan and apply.


While I was working on this I noticed that we weren't properly treating Provider objects as singletons, and thus preventing any promise coalescing for multiple accesses to results derived from provider blocks. The first commit in this PR fixes that.

I was intending to split that into a separate PR, but unfortunately these two overlap due to the second commit making use of the new providers map in Stack, so to make things less confusing and require less hoop-jumping I just combined the two together.

A Provider object owns promises for related work, so we need to make sure
we treat these as singletons to ensure that the associated work gets done
only once and all callers end up sharing the same result.

We've got away with this so far because the "related work" for providers
is all deterministic based on the configuration anyway, and so without this
we're just inefficient (doing the same work multiple times) rather than
incorrect (producing inconsistent results).

However, we should preserve the typical contract that objects containing
promises are always treated as singletons, because otherwise this
inconsistency will be a maintenance hazard for anyone who (reasonably)
expects objects of this type to be treated the same as objects of other
promise-owning types.
The "namedPromiseReporter" interface is a helper we use to collect
user-friendly names for promises when we need to report a promise
self-reference error to the user.

We collect names for promises reactively on error, rather than proactively
in the normal course of work, because proactively maintaining a suitable
data structure for this cross-cutting concern would be both expensive and
complicated due to it being a cross-cutting concern.

This compromise therefore allows us to pay the cost of walking over all
of the promise-owning objects only in the exceptional case where we need
to report a self-reference error, but it does come at the expense of it
being possible for us to forget to implement this interface for certain
types and thus be unable to name a subset of the affected promises.

This commit implements reportNamedPromises for all promise-owning types
that are reachable from a stackeval.Main, although there is a notable gap
here where we can't actually report promises from the individual instances
of a multi-instance object (e.g. a component) because the promising API
doesn't let us peek into a promising.Once to see if it's resolved without
blocking until it becomes resolved. There are FIXME comments for that
limitation in all of the places where it's relevant, and so hopefully in a
future commit we'll devise a suitable way to perform a fallible
non-blocking peek of a promise so that we can report promises nested in its
result only if they are already present.
@apparentlymart apparentlymart added the stacks Relating to the Stacks features label May 13, 2024
@apparentlymart apparentlymart requested a review from a team May 13, 2024 18:26
@apparentlymart apparentlymart self-assigned this May 13, 2024
@apparentlymart
Copy link
Member Author

apparentlymart commented May 13, 2024

The Vercel failure here does not appear to be related to the change I've made in this PR. (This change does not affect the "website" content at all.)

@apparentlymart apparentlymart merged commit 9448845 into main May 16, 2024
9 of 10 checks passed
@apparentlymart apparentlymart deleted the f-stacks-promise-names branch May 16, 2024 23:10
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

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

Successfully merging this pull request may close these issues.

None yet

2 participants