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

Introduce server.abort(request: Request) to abort an in-flight HTTP/HTTPS request #16830

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Jarred-Sumner
Copy link
Collaborator

What does this PR do?

Introduce server.abort(request: Request) to abort an in-flight HTTP/HTTPS request.

This immediately closes the TCP socket.

One subtlety here is we have to make sure that we don't call drain microtasks directly in the abort handler. Instead, we have to go through the event loop enter/exit - so that we don't drain microtasks inside of the .abort() function call itself.

How did you verify your code works?

There are tests

@robobun
Copy link

robobun commented Jan 28, 2025

Updated 1:12 AM PT - Jan 28th, 2025

@Jarred-Sumner, your commit ee239d7 has 1 failures in Build #10691:


🧪   try this PR locally:

bunx bun-pr 16830

Copy link
Member

@cirospaciari cirospaciari left a comment

Choose a reason for hiding this comment

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

this.flags.user_called_abort should be part of isAbortedOrEnded otherwise will be a footgun in future.

Also after calling abort the request_context should be detached from AnyRequestContext to avoid accessing invalid data

I would also duplicate all tests for TLS and non-TLS

expect(async () => {
await fetch(`https://localhost:${server.port}`, {
tls: {
rejectUnauthorized: false,
Copy link
Member

@cirospaciari cirospaciari Jan 28, 2025

Choose a reason for hiding this comment

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

Suggested change
rejectUnauthorized: false,
ca: tls.cert,

@Jarred-Sumner
Copy link
Collaborator Author

this.flags.user_called_abort should be part of isAbortedOrEnded otherwise will be a footgun in future.

Also after calling abort the request_context should be detached from AnyRequestContext to avoid accessing invalid data

us_socket_close callback calls the onAbort callback, which triggers the normal request abort flow. I think it's less likely to cause bugs if we can rely on the same code paths continuing to be used without changes

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.

3 participants