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.

Sign up for GitHub

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

Jump to bottom

Prevent race when resolving a property map #11186

Merged
merged 1 commit into from Oct 28, 2022
Merged

Prevent race when resolving a property map #11186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/pending/20221028--sdk-go-yaml--prevent-race-on-resource-output.yaml
View file
Open in desktop
@@ -0,0 +1,4 @@
changes:
- type: fix
scope: sdk/go,yaml
description: Prevent race on resource output
6 changes: 4 additions & 2 deletions sdk/go/pulumi/context.go
View file
Open in desktop
Expand Up @@ -1178,8 +1178,6 @@ func (state *resourceState) resolve(ctx *Context, err error, inputs *resourceInp
return
}

state.rawOutputs.getState().resolve(outprops, true, false, nil)

outprops["urn"] = resource.NewStringProperty(urn)
if id != "" || !dryrun {
outprops["id"] = resource.NewStringProperty(id)
Expand All @@ -1204,6 +1202,10 @@ func (state *resourceState) resolve(ctx *Context, err error, inputs *resourceInp
}
}

// We need to wait until after we finish mutating outprops to resolve. Resolving
// unlocks multithreaded access to the resolved value, making mutation a data race.
state.rawOutputs.getState().resolve(outprops, true, false, nil)
Copy link
Member

Choose a reason for hiding this comment

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

This makes sense to me at some basic level. resolve(outprops) shares outprops with other goroutines, but it has not finished populating in the original code. So doing it after outprops["id"]=.. stanzas seems to be better. The outprops[""] I'd love to understand what that does and what for.

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like a marker indicating either started to resolve (resource.NewObjectProperty(remaining)), finished resolving (resource.NewObjectProperty(remaining) where remaining = resource.PropertyMap{}) or will not resolve (resource.MakeComputed(resource.NewStringProperty(""))).

I'm not 100% sure though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Grepping for the key "" in sdk/go/pulumi, its only uses are in resolve function. Despite that, remove it results in test failures for TestRegisterResource.


for k, output := range state.outputs {
// If this is an unknown or missing value during a dry run, do nothing.
v, ok := outprops[resource.PropertyKey(k)]
Expand Down