Skip to content

fix(lambda-runtime,lambda-integration-tests): make spawn_graceful_shutdown_handler() async, await the extension being registered before spawning background extension handler task #992

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

Merged
merged 2 commits into from
May 16, 2025

Conversation

jlizen
Copy link
Contributor

@jlizen jlizen commented May 12, 2025

📬 Issue #, if available:
n/a

✍️ Description of changes:

Fix a race condition in spawn_graceful_shutdown_handler(). Previously function spawned a background task that handled both registration of the non-op extension and driving the extension/signal handling. This might take longer to finish registration than the primary function handler running, which would cause a crash.

Instead, we now model spawn_graceful_shutdown_handler as async and await the non-op registration completing. Updating the integration test as well.

This would normally be a semver breaking change, but we have not yet cut a release with this functionality, so it is safe.

🔏 By submitting this pull request

  • I confirm that I've ran cargo +nightly fmt.
  • I confirm that I've ran cargo clippy --fix.
  • I confirm that I've made a best effort attempt to update all relevant documentation.
  • I confirm that my contribution is made under the terms of the Apache 2.0 license.

…tdown_handler() async, await the extension being registered before spawning background extension handler task
@jlizen
Copy link
Contributor Author

jlizen commented May 15, 2025

Looking into the CI failures, there are two things going wrong:

[Build integration tests / build-runtime (stable)
Run cargo build --all-features --verbose --package lambda_integration_tests
error: cannot specify features for packages outside of workspace

You seem to have fixed this already in a separate PR, so i'll leave it be to avoid merge conflicts:
fa89ce0

error: rustc 1.81.0 is not supported by the following packages:
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
[email protected] requires rustc 1.82
Either upgrade rustc or select compatible dependency versions with
cargo update <name>@<current-ver> --precise <compatible-ver>
where <compatible-ver> is the latest version supporting rustc 1.81.0

This one is awkward. They seem to be coming from dev dependencies and integration tests only per cargo tree. I think it was due to this change on idna_adapater: hsivonen/idna_adapter@4e654c1

Going to try pinning back to an earlier minor version.

@jlizen
Copy link
Contributor Author

jlizen commented May 15, 2025

I think my commit above will address the issue, I don't have perms to run the ci/cd but my cargo tree no longer shows the breaking versions.

Some samples:

cargo tree -i -p icu_collections
icu_collections v1.5.0
├── icu_normalizer v1.5.0
│   └── idna_adapter v1.2.0
│       └── idna v1.0.3
│           └── url v2.5.4
│               ├── httpmock v0.7.0
│               │   [dev-dependencies]
│               │   └── lambda_runtime v0.13.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-runtime)
│               │       ├── aws_lambda_rust_integration_tests v0.1.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-integration-tests)
│               │       └── lambda_http v0.14.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-http)
│               │       [dev-dependencies]
│               │       └── lambda_runtime v0.13.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-runtime) (*)
│               ├── lambda_http v0.14.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-http)
│               └── reqwest v0.12.15
│                   [dev-dependencies]
│                   └── aws_lambda_rust_integration_tests v0.1.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-integration-tests)
│       [dev-dependencies]
│       └── lambda_runtime v0.13.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-runtime) (*)
└── icu_properties v1.5.1
    ├── icu_normalizer v1.5.0 (*)
    └── idna_adapter v1.2.0 (*)
zerovec v0.10.4
├── icu_collections v1.5.0
│   ├── icu_normalizer v1.5.0
│   │   └── idna_adapter v1.2.0
│   │       └── idna v1.0.3
│   │           └── url v2.5.4
│   │               ├── httpmock v0.7.0
│   │               │   [dev-dependencies]
│   │               │   └── lambda_runtime v0.13.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-runtime)
│   │               │       ├── aws_lambda_rust_integration_tests v0.1.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-integration-tests)
│   │               │       └── lambda_http v0.14.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-http)
│   │               │       [dev-dependencies]
│   │               │       └── lambda_runtime v0.13.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-runtime) (*)
│   │               ├── lambda_http v0.14.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-http)
│   │               └── reqwest v0.12.15
│   │                   [dev-dependencies]
│   │                   └── aws_lambda_rust_integration_tests v0.1.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-integration-tests)
│   │       [dev-dependencies]
│   │       └── lambda_runtime v0.13.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-runtime) (*)
│   └── icu_properties v1.5.1
│       ├── icu_normalizer v1.5.0 (*)
│       └── idna_adapter v1.2.0 (*)
├── icu_locid v1.5.0
│   ├── icu_locid_transform v1.5.0
│   │   └── icu_properties v1.5.1 (*)
│   └── icu_provider v1.5.0
│       ├── icu_locid_transform v1.5.0 (*)
│       ├── icu_normalizer v1.5.0 (*)
│       └── icu_properties v1.5.1 (*)
├── icu_locid_transform v1.5.0 (*)
├── icu_normalizer v1.5.0 (*)
├── icu_properties v1.5.1 (*)
├── icu_provider v1.5.0 (*)
└── tinystr v0.7.6
    ├── icu_locid v1.5.0 (*)
    ├── icu_locid_transform v1.5.0 (*)
    ├── icu_properties v1.5.1 (*)
    └── icu_provider v1.5.0 (*)
litemap v0.7.5
└── icu_locid v1.5.0
    ├── icu_locid_transform v1.5.0
    │   └── icu_properties v1.5.1
    │       ├── icu_normalizer v1.5.0
    │       │   └── idna_adapter v1.2.0
    │       │       └── idna v1.0.3
    │       │           └── url v2.5.4
    │       │               ├── httpmock v0.7.0
    │       │               │   [dev-dependencies]
    │       │               │   └── lambda_runtime v0.13.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-runtime)
    │       │               │       ├── aws_lambda_rust_integration_tests v0.1.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-integration-tests)
    │       │               │       └── lambda_http v0.14.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-http)
    │       │               │       [dev-dependencies]
    │       │               │       └── lambda_runtime v0.13.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-runtime) (*)
    │       │               ├── lambda_http v0.14.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-http)
    │       │               └── reqwest v0.12.15
    │       │                   [dev-dependencies]
    │       │                   └── aws_lambda_rust_integration_tests v0.1.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-integration-tests)
    │       │       [dev-dependencies]
    │       │       └── lambda_runtime v0.13.0 (/workplace/jlizen/aws-lambda-rust-runtime/lambda-runtime) (*)
    │       └── idna_adapter v1.2.0 (*)
    └── icu_provider v1.5.0
        ├── icu_locid_transform v1.5.0 (*)
        ├── icu_normalizer v1.5.0 (*)
        └── icu_properties v1.5.1 (*)

Copy link
Contributor

@bnusunny bnusunny left a comment

Choose a reason for hiding this comment

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

LGTM

@bnusunny
Copy link
Contributor

@jlizen can you check the lint error?

@jlizen
Copy link
Contributor Author

jlizen commented May 16, 2025

@bnusunny I saw the lint error, it is unrelated to my code paths. I'm glad to address it but given the number of interconnected and failing ci, maybe we could first merge the various other ci fixes to avoid muddying the water? There is one here and also one on the release branch that could be pulled out?

If we did that, I can cut a separate pr to get ci green prior to the release pr.

@bnusunny
Copy link
Contributor

Yeah, make sense

@bnusunny bnusunny merged commit 661cc26 into awslabs:main May 16, 2025
6 of 9 checks passed
@jlizen
Copy link
Contributor Author

jlizen commented May 16, 2025

@bnusunny cool, you'll merge fa89ce0 as well?

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

Successfully merging this pull request may close these issues.

2 participants