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

[pack] Ensuring JobHost restart suppresion aligns with request lifecycle #10638

Merged
merged 1 commit into from
Nov 18, 2024

Conversation

fabiocav
Copy link
Member

@fabiocav fabiocav commented Nov 17, 2024

NOTE: Additional tests and private extension tests pending.

Issue describing the changes in this PR

resolves #10631

This change aligns the restart suspension scope with the request lifecycle, ensuring that a JobHost restart triggered by changes to the functions does not resume before the request is complete, which could otherwise result in its services being disposed prematurely.

Pull request checklist

IMPORTANT: Currently, changes must be backported to the in-proc branch to be included in Core Tools and non-Flex deployments.

  • Backporting to the in-proc branch is not required
    • Otherwise: Link to backporting PR
  • My changes do not require documentation changes
    • Otherwise: Documentation issue linked to PR
  • My changes should not be added to the release notes for the next release
    • Otherwise: I've added my notes to release_notes.md
  • My changes do not need to be backported to a previous version
    • Otherwise: Backport tracked by issue/PR #issue_or_pr
  • My changes do not require diagnostic events changes
    • Otherwise: I have added/updated all related diagnostic events and their documentation (Documentation issue linked to PR)
  • I have added all required tests (Unit tests, E2E tests)

@fabiocav fabiocav requested a review from a team as a code owner November 17, 2024 18:47
@kshyju
Copy link
Member

kshyju commented Nov 18, 2024

Do we know what introduced this issue? Can we add that details to the PR description to help future readers?

@kshyju
Copy link
Member

kshyju commented Nov 18, 2024

Do we know what introduced this issue? Can we add that details to the PR description to help future readers?

Noticed in-proc backport not required. So, the DI Changes triggered this?

@fabiocav fabiocav changed the title Ensuring JobHost restart suppresion aligns with request lifecycle [pack] Ensuring JobHost restart suppresion aligns with request lifecycle Nov 18, 2024
@fabiocav
Copy link
Member Author

Do we know what introduced this issue? Can we add that details to the PR description to help future readers?

The pattern previously in place was fragile, and created an opportunity for services to be consumed after disposal. The flow of events was:

  1. API was called
  2. Restart suppressed
  3. Files written (normally, this would trigger a JobHost restart, the suppression prevented that from happening)
  4. Restart resumed, processing pending changes
  5. API call returned

When step 4 takes place, the JobHost will be terminated, and the JobHost scoped container will be disposed, causing all associated services to be disposed as well. In in-proc, although there is some evidence of the problem happening, so a backport should probably be considered, the restart didn't occur fast enough (or there were no additional service requests) to impact the response handling, with the changes in the default host, the JobHost disposal is taking place before the response is handled and no longer needs to consume services.

This was a race that was in place from the time that cod was introduced, just not something that was exposed with the previous host.

Hope the above answers the second question as well.

@kshyju
Copy link
Member

kshyju commented Nov 18, 2024

Did you miss the release notes file?

@fabiocav fabiocav force-pushed the fabiocav/restartscopechange branch from d830150 to 6540241 Compare November 18, 2024 21:10
@fabiocav fabiocav merged commit e332907 into dev Nov 18, 2024
11 checks passed
@fabiocav fabiocav deleted the fabiocav/restartscopechange branch November 18, 2024 21:39
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.

Azure Functions Host 1036 results in a 500 Error on a PUT to /admin/functions
4 participants