weaveworks / common

Libraries used in multiple Weave projects
Other
129 stars 92 forks source link

Add badResponseLoggingWriter.Flush #257

Closed leizor closed 1 year ago

leizor commented 2 years ago

This PR adds Flush() to the middleware.badResponseLoggingWriter, making it implement http.Flusher.

I think, ideally, none of this middleware should interfere with what the ultimate HTTP handlers want to try to do. Making a best-effort attempt at Flush() seems like the friendliest behavior for middleware; if it can flush, great, otherwise nothing happens.

Additionally, I think this best-effort type behavior plays nicely with this disclaimer about http.Flusher:

Note that even for ResponseWriters that support Flush, if the client is connected through an HTTP proxy, the buffered data may not reach the client until the response completes.

bboreham commented 2 years ago

It looks to me like this method will advertise the possibility of Flush() on writers that don't support it. Even though there is a disclaimer, this could break something. Hyram's Law.

What about this: create a struct embedding badResponseLoggingWriter that implements Flusher, and select which one to create in newBadResponseLoggingWriter.

leizor commented 1 year ago

I like that much better - will do!

leizor commented 1 year ago

@bboreham I think this is ready for a second look