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

use httpsnoop to ensure http.ResponseWriter additional interfaces are preserved #388

Merged
merged 4 commits into from
Oct 9, 2020

Conversation

toshok
Copy link
Contributor

@toshok toshok commented Oct 8, 2020

Noticed this problem when going through the websocket upgrade flow. http.Hijacker is implemented by builtin response writers but not by the recordingResponseWriter.

Instead of doing all the leg work to ensure that interfaces are implemented correctly, convert things to use httpsnoop, which does codegen to generate the possible combinations of interface implementations so that things are preserved.

One question: given that the recordingResponseWriter initialization code now includes an additional set of allocations (from httpsnoop, but also the closures for the hooks), is it worth it to keep the sync.Pool? My gut says no, but... ?

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Oct 8, 2020

CLA Check
The committers are authorized under a signed CLA.

go.mod Outdated Show resolved Hide resolved
@toshok
Copy link
Contributor Author

toshok commented Oct 9, 2020

hm, seeing FD exhaustion in a service running with this now. recommend holding off on accepting/landing

@toshok
Copy link
Contributor Author

toshok commented Oct 9, 2020

phew, tracked down FD leak - not anything related to this change, so should be good to go

@MrAlias
Copy link
Contributor

MrAlias commented Oct 9, 2020

@toshok This looks ready to merge given your validation. It appears maintainer edits are off for this so I cannot sync with master. Can you sync so we can merge this?

@toshok toshok force-pushed the toshok.use-httpsnoop branch from bfa74c9 to 0f783f0 Compare October 9, 2020 21:17
@toshok
Copy link
Contributor Author

toshok commented Oct 9, 2020

rebased off current master

@Aneurysm9 Aneurysm9 merged commit a84ddfe into open-telemetry:master Oct 9, 2020
@lizthegrey lizthegrey deleted the toshok.use-httpsnoop branch October 9, 2020 21:46
@pellared pellared added this to the untracked milestone Nov 8, 2024
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.

5 participants