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(cli): cdk deploy hangs when stack deployment fails #6433

Merged
merged 11 commits into from
Feb 25, 2020
Merged

fix(cli): cdk deploy hangs when stack deployment fails #6433

merged 11 commits into from
Feb 25, 2020

Conversation

shivlaks
Copy link
Contributor

@shivlaks shivlaks commented Feb 25, 2020 •
edited
Loading

Previously, when a stack would fail deployment we were using process.exit() which exited as quickly as possible, even if async operations were pending.

We rectified that by setting process.exitCode = 1 to allow all output to be flushed before a graceful exit. However, since we were not handling deployment errors, the Stack Activity Monitor was still polling every 5 seconds as the monitor is never explicitly halted.

With this change, when a stack deployment fails, we halt the monitor before throwing the error. This ensures that there are no open handles when we call process.exitCode = 1

Root caused by installing wtfnode as a dev dependency and calling wtf.dump() at the exit point. The only outstanding handle was the polling that the Stack Activity Monitor performs every 5 seconds.

Fixes #6403

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@shivlaks
fix(cli): cdk deploy hangs when stack deployment fails
49a9387 
Previously, when a stack would fail deployment we were using process.exit() which exited as quickly as possible
even if async operations were pending.

We rectified that by setting process.exitCode = 1 to allow all output to be flushed before a graceful exit.
However, since we were not handling deployment errors, the Stack Activity Monitor was still polling every 5 seconds
as the monitor is never explicitly halted.

With this change, when a stack deployment fails, we halt the monitor before throwing the error. This ensures that
there are no open handles when we call process.exitCode = 1
@shivlaks shivlaks requested a review from rix0rrr February 25, 2020 06:58
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 25, 2020
shivlaks
shivlaks commented Feb 25, 2020
View reviewed changes
Copy link
Contributor Author

@shivlaks shivlaks left a comment •
edited by iliapolo
Loading

Choose a reason for hiding this comment

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

pending:

  • Write integration test

@eladb
Copy link
Contributor

eladb commented Feb 25, 2020

plz add an integ test

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 49a9387
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 00139d7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 81109b6
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@iliapolo
Copy link
Contributor

@eladb @rix0rrr added the integ test. Notice that the test isn't really failing, it will just hang if we introduce the bug again. I imagine we have timeout configured on the build right? Are we good with this in the meantime?

I thought about using the timeout command to limit the execution, but it doesn't exist on mac and also requires some twicking to get the asserts just right. I will work on it but i think this should be fine for now so we can release.

@iliapolo iliapolo requested a review from eladb February 25, 2020 10:43
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0a84334
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 748e074
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: cc0ac87
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0ae78dd
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Feb 25, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 1394d16
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Feb 25, 2020

Thank you for contributing! Your pull request is now being automatically merged.

@mergify mergify bot merged commit 4b11d99 into master Feb 25, 2020
@mergify mergify bot deleted the shivlaks/fix-cli-hanging branch February 25, 2020 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iliapolo iliapolo iliapolo left review comments

@eladb eladb eladb left review comments

@rix0rrr rix0rrr rix0rrr approved these changes

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CDK won't exit if the deployment of nested stack failed and rollback completed
5 participants
@shivlaks @eladb @aws-cdk-automation @iliapolo @rix0rrr