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

Fix a hang in nodejs remote components when an error is thrown within an apply #7365

Merged
merged 4 commits into from
Jun 26, 2021
Merged

Fix a hang in nodejs remote components when an error is thrown within an apply #7365

merged 4 commits into from
Jun 26, 2021

Conversation

EvanBoyle
Copy link
Contributor

Description

The uncaught handler wasn't terminating the outstanding gRPC request causing a hang. This change adds/removes an uncaught handler around constructImpl calls that bubbles the uncaught error back via the gRPC callback.

Fixes # #6991

Checklist

  • I have added tests that prove my fix is effective or that my feature works

@EvanBoyle EvanBoyle mentioned this pull request Jun 25, 2021
Closed
justinvp
justinvp approved these changes Jun 25, 2021
View reviewed changes
Copy link
Member

@justinvp justinvp left a comment

Choose a reason for hiding this comment

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

LGTM

tests/integration/integration_nodejs_test.go Outdated
Comment on lines 1024 to 1026
if runtime.GOOS == WindowsOS {
t.Skip("Temporarily skipping test on Windows")
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we try running it on Windows?

You'll need to update build.proj like:

pulumi/build.proj

Lines 283 to 286 in 634e97c

<Exec Command="yarn install"
WorkingDirectory="$(TestsDirectory)\integration\construct_component_unknown\testcomponent" />
<Exec Command="yarn link @pulumi/pulumi"
WorkingDirectory="$(TestsDirectory)\integration\construct_component_unknown\testcomponent" />

and

pulumi/build.proj

Line 298 in 634e97c

<Exec Command="yarn run tsc" WorkingDirectory="$(TestsDirectory)\integration\construct_component_unknown\testcomponent" />

sdk/nodejs/provider/server.ts Outdated Show resolved Hide resolved
tests/integration/construct_component_error_apply/nodejs/component.ts Outdated Show resolved Hide resolved
tests/integration/construct_component_error_apply/nodejs/index.ts Outdated Show resolved Hide resolved
tests/integration/construct_component_error_apply/testcomponent/index.ts Show resolved Hide resolved
@EvanBoyle
Copy link
Contributor Author

Flakey codegen test is unrelated. Merging.

@EvanBoyle EvanBoyle merged commit c37cbc9 into master Jun 26, 2021
@pulumi-bot pulumi-bot deleted the evan/nodejsConstructApplyHang branch June 26, 2021 01:41
@EvanBoyle EvanBoyle mentioned this pull request Jun 28, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinvp justinvp justinvp approved these changes

@komalali komalali Awaiting requested review from komalali

@t0yv0 t0yv0 Awaiting requested review from t0yv0

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@EvanBoyle @justinvp