caddy-prometheus icon indicating copy to clipboard operation
caddy-prometheus copied to clipboard

Using this plugin with websockets causes a panic

Open wade-p opened this issue 7 years ago • 10 comments

from the error log: 26/May/2018:23:46:16 +0000 [PANIC /<PATH>] caddyhttp/proxy/reverseproxy.go:352 - *metrics.timedResponseWriter is not a hijacker

I think that can be fixed by implementing the Hijacker interface: https://golang.org/src/net/http/server.go#L174

It looks like it could be similar to how WriteHeader works and just call and return Hijack after calling w.didWrite()

https://golang.org/src/net/http/server.go#L1883

wade-p avatar May 27 '18 00:05 wade-p

I think #42 fixes this

hairyhenderson avatar May 27 '18 15:05 hairyhenderson

Found it eigther

[PANIC] *metrics.timedResponseWriter is not a hijacker

Thats really annoying, that this request crashed.

@miekg Whats the status ?

genofire avatar Oct 19 '18 09:10 genofire

[ Quoting [email protected] in "Re: [miekg/caddy-prometheus] Using ..." ]

Found it eigther

[PANIC] *metrics.timedResponseWriter is not a hijacker

Thats really annoying, that this request crashed.

@miekg Whats the status ?

zero - i don't have time to work on this

miekg avatar Oct 19 '18 09:10 miekg

@genofire are you able to try the patch in #42 to see if it fixes the bug for you?

(note that I also don't have time to look deeply into this, but the description mentioned implementing http.Hijacker as a potential fix, which is what #42 is about)

hairyhenderson avatar Oct 19 '18 15:10 hairyhenderson

Additionally, here is the patch I wrote which has been working well for me.

index 6d40684..fde1810 100644
--- a/handler.go
+++ b/handler.go
@@ -1,6 +1,7 @@
 package metrics

 import (
+       "bufio"
        "net"
        "net/http"
        "strconv"
@@ -92,6 +93,7 @@ func isIPv6(addr string) bool {
 type timedResponseWriter struct {
        firstWrite time.Time
        http.ResponseWriter
+       http.Hijacker
 }

 func (w *timedResponseWriter) didWrite() {
@@ -111,3 +113,8 @@ func (w *timedResponseWriter) WriteHeader(statuscode int) {
        w.didWrite()
        w.ResponseWriter.WriteHeader(statuscode)
 }
+
+func (w *timedResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
+       w.didWrite()
+       return w.Hijacker.Hijack()
+}

wade-p avatar Oct 20 '18 05:10 wade-p

same for us

BobCashStory avatar Jun 01 '19 01:06 BobCashStory

I am get

*metrics.timedResponseWriter is not a closeNotifier

Implement the CloseNotifier interface. To solve my problem.

lou-lan avatar Aug 06 '19 11:08 lou-lan

Adding feedback for this open issue, I hadn't previously experienced a problem using the prometheus module with websockets (works fine for Grafana), until I needed to downgrade the HTTP version to http/1.1 (working around an issue with Portainer's console access):

{$CADDY_URL}:9000 {
  log / stdout "{combined}"
  errors stderr
  #prometheus 0.0.0.0:9180 {
  #  hostname 9000
  #}
  tls {$CADDY_CERT} {$CADDY_KEY} {
    alpn http/1.1
  }

  proxy / portainer:9000 {
    transparent
    websocket
  }
}

Enabling the prometheus module above results in caddy log output:

[email protected] | 2019/08/10 13:49:22 [PANIC] *metrics.timedResponseWriter is not a hijacker

mh720 avatar Aug 10 '19 14:08 mh720

hotfix, see https://github.com/miekg/caddy-prometheus/pull/42, it is work

lou-lan avatar Aug 10 '19 14:08 lou-lan

Tested in Docker (https://github.com/swarmstack/caddy). Summited a PR to #42 patch author to update github repos from mholt to caddyserver.

mh720 avatar Aug 13 '19 03:08 mh720