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

fix(http): panic when responding to a closed conn #12216

Merged
merged 5 commits into from
Sep 25, 2021

Conversation

AaronO
Copy link
Contributor

@AaronO AaronO commented Sep 24, 2021

Our oneshot receiver in HyperService::call would unwrap and panic, the .await on the oneshot receiver errors when the sender is dropped.

The sender is dropped in op_http_response because:

  1. We take ResponseSenderResource (which owns the sender)
  2. Then we hit an early exit in op_http_response due to the conn being closed
  3. Then the taken sender gets dropped in this early exit before any response is sent over the channel

Fallbacking to returning a dummy response to hyper seems to be a fine quickfix

Our oneshot receiver in `HyperService::call` would unwrap and panic, the `.await` on the oneshot receiver happens when the sender is dropped.

The sender is dropped in `op_http_response` because:
1. We take `ResponseSenderResource`
2. Then get `ConnResource` and early exit on failure (conn already closed)
3. The taken sender then gets dropped in this early exit before any response is sent over the channel

Fallbacking to returning a dummy response to hyper seems to be a fine quickfix
@piscisaureus
Copy link
Member

Fix LGTM, is it possible to add a test for this?

A stand-alone test is here: https://github.com/denoland/deploy/issues/1949#issuecomment-926873141

Copy link
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

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

It seems hard to reliably trigger just this bug without sometimes triggering another issue.
I don’t want to land flaky tests.
And I like bugs to be fixed one at a time.
Therefore I think we can ship this as-is.

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.

We should have a test. Even a flaky test is better than none - tho a solid one would be great.

@AaronO
Copy link
Contributor Author

AaronO commented Sep 25, 2021

We should have a test. Even a flaky test is better than none - tho a solid one would be great.

I've added a non-flaky test.

You can run it yourself:

deno run -A https://gist.githubusercontent.com/AaronO/fc7fe0aed81afa4706218df7fd97b62e/raw/684851feaeb0216d592400c882a439645648fa53/deno_sender_panic_reprod.js

@AaronO AaronO requested a review from ry September 25, 2021 10:42
must have non-empty catch branch
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 - nice work!

@AaronO AaronO merged commit 3c88dff into denoland:main Sep 25, 2021
@AaronO AaronO deleted the fix/http-panic-respond-closed-conn branch September 25, 2021 11:22
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.

None yet

3 participants