projects like chi v4 deprecate http.CloseNofity and stop working for grpc-web

[wuyuanyi135]

Hi,
I am currently evaluating the grpc frameworks for my next front-end project. I ran into a weird bug this evening. Everything worked in the example project, but the same concept did not work in my project:

2019/02/09 00:35:06 http: panic serving [::1]:43736: interface conversion: *middleware.httpFancyWriter is not http.CloseNotifier: missing method CloseNotify
goroutine 4 [running]:
...
*****/improbable-eng/grpc-web/go/grpcweb.(*grpcWebResponse).CloseNotify(0xc00014c4e0, 0x9e3ac0)
	*****/improbable-eng/grpc-web/go/grpcweb/grpc_web_response.go:52 +0x44

I played around it, and found commenting out this line the error disappeared.
Then, I noticed that go-chi has just gotten a major update (v3->v4), and they are deprecating http.CloseNotifier see this commit. Since this middleware replaces the ResponseWriter to an object without http.CloseNotifier, it breaks the call.

I hope there is a clarification for the example project that the latest chi is not compatible.
Will this project deprecate this API soon?

[johanbrandhorst]

I'm not sure but I think the CloseNotifier is still a requirement of the gRPC.ServeHTTP API. Did you say it was all working without this line in this library code? If you would submit a PR with the changes and we can get the tests passing I don't see why we couldn't remove it though.

[RXminuS]

what's the status of this @wuyuanyi135, why was this closed? It still seems to be a problem

[johanbrandhorst]

@RXminuS thanks for reporting this, would you be interested in contributing a fix for this?

[mangas]

I've just encountered this problem as well. It looks like under src/github.com/improbable-eng/grpc-web/go/grpcweb/grpc_web_response.goline 69 assumes that it is safe to cast the wrapped interface to http.ResponseWriter which in turn doesn't implement the interface anymore.

Looking at it the quick fix seems like it could be just add a switch case but I'm not sure if that wouldn't have wider implications. Any thoughts? @johanbrandhorst

[johanbrandhorst]

I think we can use the precedent set by grpc-go (grpc/grpc-go#2694) and simply remove these methods. @mangas would you be interested in contributing this fix?

[mangas]

That seems like a much bigger change, would you fine with me proposed solution?

[johanbrandhorst]

I don't see how you could implement this with a type switch. What would you return if it doesn't implement it? A nil channel would potentially crash the program instead. Just remove the function.

[mangas]

I was going to add a channel to the struct that we'd return in that case. The function is being called, I assume that comes from the actual grpc-server implementation.

[johanbrandhorst]

The gRPC server doesn't use the method. It has been deprecated in Go since context was introduced in 1.7 (I think?). We should just remove it from our methods now that we have several releases since the deprecation.

[mangas]

It seems like Flusher is another issue, are you aware of a bug report for it? Same situation, interface cast without a check as far as I understand it

[johanbrandhorst]

Flusher is not deprecated, so we can't remove that, and we probably still want to support that. We could perhaps consider a type switch there, but is there actually a problem? If you find an issue with it (other than just a suspicion from looking at the code), please raise a separate issue.

[mangas]

Yeah, will do. I'm still investigating, the. issue is come from the usage of a wrapper library that provides tracing capabilities but only implements the standard http.ResponseWriter interfaces