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

Suppress aborthandler #21479

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

Conversation

ianseyer
Copy link

@ianseyer ianseyer commented Feb 3, 2025

Fixes #18087, #15751, #10813, #12011, #16556, #20148

As per golang/go#28239, it is the expectation that middleware using net/http is responsible for handling http.AbortHandler panics that are automatically recovered.

In the current situation, any network connectivity blip, despite recovering successfully, results in a panic being written to the logs.

I have tested this build in our staging environment, and the panic message is indeed suppressed. We do see a proxy connection interrupted log message, which is to be expected (we have a reliable source of network connectivity blips).

Please indicate you've done the following:

  • Well Written Title and Summary of the PR
  • Label the PR as needed. "release-note/ignore-for-release, release-note/new-feature, release-note/update, release-note/enhancement, release-note/community, release-note/breaking-change, release-note/docs, release-note/infra, release-note/deprecation"
  • Accepted the DCO. Commits without the DCO will delay acceptance.
  • Made sure tests are passing and test coverage is added if needed.
  • Considered the docs impact and opened a new docs issue or PR with docs changes if needed in website repository.

dependabot bot and others added 2 commits February 3, 2025 10:56
…src (goharbor#21162)

Bumps [go.opentelemetry.io/otel](https://github.com/open-telemetry/opentelemetry-go) from 1.31.0 to 1.32.0.
- [Release notes](https://github.com/open-telemetry/opentelemetry-go/releases)
- [Changelog](https://github.com/open-telemetry/opentelemetry-go/blob/main/CHANGELOG.md)
- [Commits](open-telemetry/opentelemetry-go@v1.31.0...v1.32.0)

---
updated-dependencies:
- dependency-name: go.opentelemetry.io/otel
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: miner <[email protected]>
@ianseyer ianseyer requested a review from a team as a code owner February 3, 2025 17:06
@Vad1mo Vad1mo added the release-note/update Update or Fix label Feb 3, 2025
@Vad1mo
Copy link
Member

Vad1mo commented Feb 5, 2025

Thank you for your contribution, this is a quite common issue, we have also seen ourself.

case r == nil:
return

case ok && errors.Is(err, http.ErrAbortHandler):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be good to print a log with this error, just in case there are very frequent network connection issues.

Copy link
Author

@ianseyer ianseyer Feb 6, 2025

Choose a reason for hiding this comment

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

So there will, at least in my testing, still be http: proxy error: context canceled emitted if the connection is interrupted.

It is also worth mentioning that in net/http/server, this type of error is completely ignored: https://github.com/golang/go/blob/master/src/net/http/server.go#L1944

@ianseyer
Copy link
Author

@reasonerjt what is the follow-up required?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handler crashed with error net/http: abort Handler
5 participants