Check flusher interface before calling Flush

[mangas]

FIxes #480

Changes

Ensure the cast succeeds before calling method.

Verification

Behaviour is unchanged for the standard case

[johanbrandhorst]

Small suggestion. I'm in two minds about this, because on the one hand this risks us silently supporting ResponseWriters that won't work. I think we need to have a discussion around this before agreeing to merge it. Why do you need this?

[mangas]

I think the question here should be "why are we castinghttp.ResponseWriter to http.Flusher when the default implementation doesn't have that? Unless I'm missing something?

[johanbrandhorst]

The default implementation (the net/http ResponseWriter) type does support Flush (See https://golang.org/pkg/net/http/#Flusher). I agree that it might've been wrong to blindly assume it when the interface we're using doesn't have it., but now we're using it, not having it available might produce invalid output.

[mangas]

https://golang.org/pkg/net/http/#ResponseWriter It's not on the default interface

[johanbrandhorst]

No, it's not on the default interface, because they couldn't change the interface for backwards-compatibility reasons, but as you can see on https://golang.org/pkg/net/http/#Flusher:

The default HTTP/1.x and HTTP/2 ResponseWriter implementations support Flusher, but ResponseWriter wrappers may not. Handlers should always test for this ability at runtime.

[mangas]

"The Flusher interface is implemented by ResponseWriters that allow an HTTP handler to flush buffered data to the client. "

The ones that allow, not necessarily all of them

[mangas]

from that quote as well:

The default HTTP/1.x and HTTP/2 ResponseWriter implementations support Flusher, but ResponseWriter wrappers may not. Handlers should always test for this ability at runtime.

This is exactly what I'm proposing, checking for the ability before calling it

[johanbrandhorst]

I'm not sure what your point is, my point is that this library might not be able to support ResponseWriters that don't implement http.Flusher, because we've never tested it on one. Because the default http response implementations implement http.Flusher, and most handler wrappers will be using the default implementations under the hood, they should be able to support it as well.

[johanbrandhorst]

If we merge this, it might produce incorrect output, because the current implementation relies on being able to call Flush explicitly. I think failing silently, like this change would do, is really dangerous, because a user might get some weird behaviour unexpectedly, whereas currently it will be very obvious that your ResponseWriter doesn't support Flusher, because your application crashed.

Before we merge this I would ask that you test this in your environments and confirm that it still works as expected.

[mangas]

For me it solves the problem, but then again we were not expecting Flusher implementations

[johanbrandhorst]

I would encourage you to try to get a ResponseWriter that also implements Flusher. What are you using at the moment that doesn't support it?

[mangas]

github.com/opentracing-contrib's MIddleware

[johanbrandhorst]

Sounds like you could just contribute a Flush method to this?

[johanbrandhorst]

Wait, are you talking about https://github.com/opentracing-contrib/go-stdlib/blob/master/nethttp/status-code-tracker.go? It supports flusher as far as I can tell.

[mangas]

It only returns the types the original writer supports. I'm still a bit confused, the official docs suggest that this behaviour should be checked for. Would adding some logging help in this case? Or if the Flusher is required then that should be the enforced type to make it absolutely obvious that it is a requirement, although that doesn't seem to be the case either.

[johanbrandhorst]

No I agree, maybe the best thing would be for us to change the dependency to one that includes the Flusher. What do you think about that?

[mangas]

That would definitely make the API more clear and prevent these runtime errors by providing some type safety.

[johanbrandhorst]

It would technically be a backwards compatibility breaking change but as you noticed, any existing users of the narrower interface will have had a bad time 😁. Would you be interested in contributing this fix?

[mangas]

I'm actually still wondering if some kind of logging wouldn't solve the problem since Flush is not really needed for all scenarios. I could come around to do it but at the moment I kinda need to fix the issue of it not working for me so if someone wants to pick it up it's up for grabs :)

[johanbrandhorst]

Do we have a logger already, or do we use the grpclog library? I can't remember. I think if we really need this we can probably just log a warning when flush is called, as you say.

[jonny-improbable]

@johanbrandhorst, @mangas what's the next step for this PR?

[johanbrandhorst]

I think @mangas has abandoned this effort, but given the real problem, I recommend we merge this anyway. We can make the suggested changes if any consumers are complaining about the changes. We should make a new "major" release though (0.x+1.0).

[mangas]

Hey @johanbrandhorst, at the time I ended up sorting this by upgrading another library that took care of exposing the Flusher interfaces. Ended up forgetting to write an update.

[johanbrandhorst]

Unfortunately this caused #526, so I will have to back this out again. @jonnyreeves we probably have to prepare v0.10.1 with the revert in place.

[johanbrandhorst]

Hi @mangas, would you be interested in re-submitting this fix? Unfortunately the first one had a critical bug that wasn't discovered until a user ran into it 😱. It's our fault as much as yours so this is not an attempt to appoint blame. We'd still like to see this functionality as it was causing a problem for you originally.

[kostyay]

Hey
Any progress with this or was this project abandoned?
I'm also facing this problem.

[johanbrandhorst]

The project is not abandoned, but I don't know if this has been fixed yet. Would you be willing to contribute a fix?

[kostyay]

The project is not abandoned, but I don't know if this has been fixed yet. Would you be willing to contribute a fix?

https://github.com/improbable-eng/grpc-web/pull/817/files