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

chore(ext/http): add "legacy_abort" feature flag #28371

Merged
merged 12 commits into from
Mar 22, 2025

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Mar 3, 2025

Deno.serve Request abort signals are aborted by default even when it is finished successfully. This PR gates this behavior behind the "legacy_abort" which is the default right now.

Turning the no_legacy_abort runtime option on is a breaking change and will only abort request signals when there is a failure, thereby cannot be used to determine if the request finished. This aligns with fetch API.

Fixes #27005

@ry ry added this to the 2.3.0 milestone Mar 3, 2025
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

  • Add feature flag, global set on startup
  • test?

@littledivy littledivy changed the title BREAKING(ext/http): don't abort request signal on success chore(ext/http): add "legacy_abort" feature flag Mar 7, 2025
x
@littledivy littledivy requested review from ry and lucacasonato March 7, 2025 16:28
Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

I think a runtime feature flag is better

x
x
Comment on lines +135 to +136
/// If `false`, the server will abort the request when the response is dropped.
pub no_legacy_abort: bool,
Copy link
Member

Choose a reason for hiding this comment

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

When is this set to true? Ideally it would be an unstable feature flag

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

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

LGTM - Please land with a descriptive commit message about the feature flag and default value and the fact that this is a breaking change and hence we're being careful about it.

@littledivy littledivy merged commit 48ccb67 into denoland:main Mar 22, 2025
18 checks passed
littledivy added a commit that referenced this pull request Mar 28, 2025
Deno.serve `Request` abort signals are aborted by default even when it
is finished successfully. This PR gates this behavior behind the
"legacy_abort" which is the default right now.

Turning the `no_legacy_abort` runtime option on is a **breaking change**
and will only abort request signals when there is a failure, thereby
cannot be used to determine if the request finished. This aligns with
`fetch` API.

Ref #27005
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.

Deno.serve: request.signal is aborted even though the response finished successfully
2 participants