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

Update ContextReference & DefaultFromBodyConversionFeature to remove invocation cancellation token usage #2894

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

liliankasem
Copy link
Member

@liliankasem liliankasem commented Dec 10, 2024

Issue describing the changes in this PR

resolves #2417

Pull request checklist

  • 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
  • I have added all required tests (Unit tests, E2E tests)

Additional information

  1. The ContextReference to remove and reference or use of the invocation cancellation token
  2. The DefaultFromBodyConversionFeature to remove and reference or use of the invocation cancellation token

Issue

Currently, ContextReference registers the invocation’s cancellation token to multiple internal tasks and value sources. One of these, _functionCompletionTask, is responsible for disposing of the HttpContext. When the invocation is canceled, this results in the HttpContext being disposed prematurely, leading to an ObjectDisposedException when the function attempts to complete the HTTP response.

We have reason to believe that canceling _functionCompletionTask propagates cancellation through the ASP.NET Core pipeline, causing the HttpContext to be disposed earlier than expected. Since HttpContext is typically disposed of naturally when the request completes, this explicit disposal is unnecessary and introduces unwanted side effects.

Proposed Change

This PR removes the cancellation token registration from ContextReference, ensuring that:

  • Function executions are not forcefully canceled when the invocation is aborted.
  • HttpContext remains available until the invocation naturally completes.
  • Cancellation behavior aligns with ASP.NET Core, where execution continues unless explicitly handled by user code.

By following the design principle that framework code should propagate cancellation but never react to it, this change ensures that handling cancellations remains the responsibility of customer code while preventing unintended HttpContext disposal.

Impact

With this change, function executions will continue running even if the client disconnects, which is the default behavior in ASP.NET Core. Users who want to cancel function execution explicitly can still do so by checking the CancellationToken within their function logic.

_functionStartTask and _functionCompletionTask

These tasks will no longer be canceled automatically when the cancellation token is triggered. This means that if a client disconnects or the invocation is canceled, these tasks will not be marked as canceled, and they will continue to track the function’s start and completion until the function completes. I believe this is behaviour we want.

_httpContextValueSource and _functionContextValueSource

Removing the cancellation token registration will prevent these value sources from being canceled automatically when the cancellation token is triggered. This means that HttpContext and FunctionContext will not be disposed of prematurely, allowing them to remain available until the function invocation completes. I believe this is behaviour we want.

This change is beneficial because it treats canceled invocations just like regular invocations, without any special handling. This gives customers the flexibility to manage cancellation in a way that best fits their needs.

@liliankasem liliankasem force-pushed the liliankasem/invocation-ct branch from 627a808 to 70644de Compare December 11, 2024 19:22
@liliankasem liliankasem changed the title Don't use the invocation CT to cancel context ref tasks Changes to ensure the worker only propagates the invocation CT, and does not react to it Dec 11, 2024
@liliankasem liliankasem force-pushed the liliankasem/invocation-ct branch from 68a5025 to ac93ee7 Compare December 11, 2024 22:25
@liliankasem liliankasem changed the title Changes to ensure the worker only propagates the invocation CT, and does not react to it Update ContextReference & DefaultFromBodyConversionFeature to remove invocation cancellation token usage Dec 12, 2024
@liliankasem liliankasem marked this pull request as ready for review December 12, 2024 23:20
Copy link
Contributor

@jviau jviau left a comment

Choose a reason for hiding this comment

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

Approving. If you have any formal explanation for why removing that token registration in ContextReference is a good change that would be helpful.

I know I suggested that change 😆 but it is still a significant behavior change, wondering what the impact of it really is.

@liliankasem
Copy link
Member Author

Approving. If you have any formal explanation for why removing that token registration in ContextReference is a good change that would be helpful.

I know I suggested that change 😆 but it is still a significant behavior change, wondering what the impact of it really is.

I may have something in the design doc! I'll fill in any blanks and can update the PR desc with more info

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.

ObjectDisposedException: Request has finished and HttpContext disposed. in Extensions.Http.AspNetCore
3 participants