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

Explicity write http headers on streaming endpoints #47796

Merged
merged 1 commit into from
May 16, 2024

Conversation

cpuguy83
Copy link
Member

@cpuguy83 cpuguy83 commented May 3, 2024

This works around issues with the otel http handler wrapper causing multiple calls to WriteHeader when a Flush is called before Write.

closes #47448

Prevent daemon log being spammed with `superfluous response.WriteHeader call ...` message.

@vvoland
Copy link
Contributor

vvoland commented May 14, 2024

Added an impact/changelog label as it's an user visible bugfix.

daemon/stats.go Outdated Show resolved Hide resolved
api/server/router/system/system_routes.go Show resolved Hide resolved
@cpuguy83 cpuguy83 force-pushed the fix_superfluous_write_header branch from f30c263 to b19aca4 Compare May 16, 2024 16:39
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

I think @krissetto also wanted to have a look

This works around issues with the otel http handler wrapper causing
multiple calls to `WriteHeader` when a `Flush` is called before `Write`.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83 cpuguy83 force-pushed the fix_superfluous_write_header branch from b19aca4 to 707ab48 Compare May 16, 2024 18:00
Copy link
Contributor

@krissetto krissetto left a comment

Choose a reason for hiding this comment

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

seems good to me.
shall we close #47715 and #47718 once this is merged?

@thaJeztah
Copy link
Member

#47718 could still be something to consider if we don't consider the package to be something for external use; WDYT @cpuguy83 ?

@cpuguy83
Copy link
Member Author

I think its fine to move it to internal, however I'm not sure about the updated implementation.

@thaJeztah thaJeztah merged commit 06e3a49 into moby:master May 16, 2024
126 checks passed
@cpuguy83 cpuguy83 deleted the fix_superfluous_write_header branch May 16, 2024 21:27
@krissetto
Copy link
Contributor

I think its fine to move it to internal, however I'm not sure about the updated implementation.

I'd have to get rid of the changes made to workaround this superfluous writeheader issue, but the rest of the implementation has been left as-is (with one exception, the removal of a func that had a comment saying it really shouldn't be used)

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.

Syslog entry: "superfluous response.WriteHeader call" in Docker 25.x
4 participants