[fix] `httpbp` middleware doesn't flush chunked responses #573

adamthesax · 2022-10-24T18:01:00Z

Closes #

💸 TL;DR

httpbp middleware, in particular RecordStatusCode and PrometheusServerMetrics don't work with chunked HTTP responses because they wrap http.ResponseWriter and do not implement http.Flush. This updates our response wrapping within in the Middleware to support both the http.Flush and http.Hijacker interface.

📜 Details

Design Doc

Jira

🧪 Testing Steps / Validation

✅ Checks

CI tests (if present) are passing
Adheres to code style for repo
Contributor License Agreement (CLA) completed if not a Reddit employee

kylelemons

Thanks!

I think the only thing I would request is to make a unit test that makes a dummy server with the default middleware chain and makes sure that the handler can be hijacked and (separately) flushed.

httpbp/middlewares.go

Co-authored-by: Kyle Lemons <[email protected]>

httpbp/middlewares_test.go

fishy · 2022-10-24T22:25:01Z

httpbp/middlewares.go

+type wrappedHijacker struct {
+	http.ResponseWriter
+	http.Hijacker
+}
+
+type wrappedFlusher struct {
+	http.ResponseWriter
+	http.Flusher
+}
+
+type wrappedFlushHijacker struct {
+	http.ResponseWriter
+	http.Flusher
+	http.Hijacker
+}
+
+func allowFlushHijack(original, rw http.ResponseWriter) http.ResponseWriter {
+	flusher, isFlusher := original.(http.Flusher)
+	hijacker, isHijacker := original.(http.Hijacker)
+	switch {
+	case isFlusher && isHijacker:
+		return &wrappedFlushHijacker{rw, flusher, hijacker}
+	case isFlusher:
+		return allowFlush(original, rw)
+	case isHijacker:
+		return allowHijack(original, rw)
+	default:
+		return rw
+	}
+
+}


🔕 it's manage-able as we currently have 2 optional interfaces, it will become a nightmare if there's a 3rd one added in the future. but I also can't find a better way to do this.

Yeah this approach is definitely verbose but I can't think of a better one. The only other interface on http that a http.ResponseWriter could implement would be http.Pusher. I don't think it's worth supporting it now as I don't think we have any use case driving it, but that would be a 3rd interface that may come up in the future.

This is a quietly known issue amongst the ecosystem, and the approach in the generic case seems to be code generation.

https://www.doxsey.net/blog/fixing-interface-erasure-in-go/

Thanks for the article! I updated this PR to use the approach outlined in the article and included http.Pusher. Because we are dealing with 3 interfaces, I adapted their code generator and then massaged the output to make it more human readable. Basically leveraging the jump table idea, without having to adapt a full code generator.

@kylelemons let me know if you think is approach is better or if we should revert back to my previous one.

kylelemons

I personally prefer the previous approach. It's true that Pusher is missed, but it has the benefit that the type names wouldn't be atrocious :)

That said, I have no issue with keeping this. The approach is sound, and it's more complete.

kylelemons · 2022-10-27T18:24:06Z

httpbp/response_wrappers.go

@@ -0,0 +1,33 @@
+// DO NOT EDIT.


DO NOT EDIT is incompatible with "this was generated and then edited" :D

+1.

I would say it's better to just hand write this code:

type optionalResponseWriter int const ( flusher optionalResponseWriter = 1 << iota hijacker pusher ) func wrapResponseWriter(...) ... { var f optionalResponseWriter ... switch f { case 0: ... case flusher: ... case flusher | hijacker: ... ... }

fishy

I don't have a strong preference between this and the previous approach, but if we pick this approach, I'd prefer to hand writing the code over using codegen.

fishy · 2022-10-27T21:48:06Z

httpbp/response_wrappers.go

@@ -0,0 +1,33 @@
+// DO NOT EDIT.


+1.

I would say it's better to just hand write this code:

type optionalResponseWriter int const ( flusher optionalResponseWriter = 1 << iota hijacker pusher ) func wrapResponseWriter(...) ... { var f optionalResponseWriter ... switch f { case 0: ... case flusher: ... case flusher | hijacker: ... ... }

httpbp/response_wrappers.go

wrapped middleware supports Flush() and Hijack()

97c1121

adamthesax force-pushed the fix-add-flush-to-httpbp-middleware branch from c13ff31 to 97c1121 Compare October 24, 2022 18:56

adamthesax changed the title ~~[fix] httpbp middleware breaks chunked reponses~~ [fix] httpbp middleware doesn't flush chunked responses Oct 24, 2022

kylelemons requested review from fishy and kylelemons October 24, 2022 19:54

kylelemons requested changes Oct 24, 2022

View reviewed changes

httpbp/middlewares.go Outdated Show resolved Hide resolved

adamthesax and others added 2 commits October 24, 2022 15:56

Update httpbp/middlewares.go

49397ad

Co-authored-by: Kyle Lemons <[email protected]>

added unit test

7a442bc

adamthesax marked this pull request as ready for review October 24, 2022 21:17

adamthesax requested a review from a team as a code owner October 24, 2022 21:17

adamthesax requested review from pacejackson and removed request for a team October 24, 2022 21:17

chain wrapper functions together

08e1879

adamthesax requested review from kylelemons and removed request for fishy and pacejackson October 24, 2022 21:24

test tweaks

193243d

pacejackson approved these changes Oct 24, 2022

View reviewed changes

fishy approved these changes Oct 24, 2022

View reviewed changes

additional test cases

5ad3f18

fishy approved these changes Oct 25, 2022

View reviewed changes

update to use jump table and support http.Pusher

f3f28de

kylelemons approved these changes Oct 27, 2022

View reviewed changes

kylelemons requested a review from fishy October 27, 2022 21:43

fishy requested changes Oct 27, 2022

View reviewed changes

remove generated code and write all wrappers by hand

574ce14

adamthesax requested a review from fishy October 31, 2022 15:58

fishy approved these changes Oct 31, 2022

View reviewed changes

httpbp/response_wrappers.go Outdated Show resolved Hide resolved

PR feedback

022d7f1

fishy approved these changes Oct 31, 2022

View reviewed changes

kylelemons merged commit fae00cb into reddit:master Nov 1, 2022

fishy mentioned this pull request Nov 1, 2022

Metrics cleanups in other packages #582

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] `httpbp` middleware doesn't flush chunked responses #573

[fix] `httpbp` middleware doesn't flush chunked responses #573

adamthesax commented Oct 24, 2022 •

edited

Loading

kylelemons left a comment •

edited

Loading

fishy Oct 24, 2022

adamthesax Oct 25, 2022

kylelemons Oct 25, 2022

adamthesax Oct 26, 2022

kylelemons left a comment

kylelemons Oct 27, 2022

fishy Oct 27, 2022 •

edited

Loading

fishy left a comment

fishy Oct 27, 2022 •

edited

Loading

[fix] httpbp middleware doesn't flush chunked responses #573

[fix] httpbp middleware doesn't flush chunked responses #573

Conversation

adamthesax commented Oct 24, 2022 • edited Loading

💸 TL;DR

📜 Details

🧪 Testing Steps / Validation

✅ Checks

kylelemons left a comment • edited Loading

Choose a reason for hiding this comment

fishy Oct 24, 2022

Choose a reason for hiding this comment

adamthesax Oct 25, 2022

Choose a reason for hiding this comment

kylelemons Oct 25, 2022

Choose a reason for hiding this comment

adamthesax Oct 26, 2022

Choose a reason for hiding this comment

kylelemons left a comment

Choose a reason for hiding this comment

kylelemons Oct 27, 2022

Choose a reason for hiding this comment

fishy Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

fishy left a comment

Choose a reason for hiding this comment

fishy Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

[fix] `httpbp` middleware doesn't flush chunked responses #573

[fix] `httpbp` middleware doesn't flush chunked responses #573

adamthesax commented Oct 24, 2022 •

edited

Loading

kylelemons left a comment •

edited

Loading

fishy Oct 27, 2022 •

edited

Loading

fishy Oct 27, 2022 •

edited

Loading