search

igm / sockjs-go

WebSocket emulation - Go server library
BSD 3-Clause "New" or "Revised" License
514 stars 100 forks source link

CloseNotify issue. #60

Closed wavded closed 6 years ago

wavded commented 6 years ago

Running against latest master and on go version go1.9.2 linux/amd64, I get the following panic. I don't know what causes it yet, perhaps xhr? Any help would be really appreciated as it only seems to show up in production :(

panic: net/http: CloseNotify called after ServeHTTP finished
goroutine 4804667 [running]:
net/http.(*response).CloseNotify(0xc42361a000, 0x9b4240)
#011/usr/lib/go-1.8/src/net/http/server.go:1902 +0xa3
/vendor/github.com/urfave/negroni.(*responseWriterCloseNotifer).CloseNotify(0xc42420a008, 0xc422d182a0)
#011/vendor/github.com/urfave/negroni/response_writer.go:112 +0x5a
/vendor/github.com/igm/sockjs-go/sockjs.newHTTPReceiver.func1(0x7ffb3d6dc530, 0xc42420a008, 0xc420ad85f0)
#011/vendor/github.com/igm/sockjs-go/sockjs/httpreceiver.go:46 +0x49
created by /vendor/github.com/igm/sockjs-go/sockjs.newHTTPReceiver
#011/vendor/github.com/igm/sockjs-go/sockjs/httpreceiver.go:57 +0x172
wavded commented 6 years ago

I do use negroni but I think the issue would occur regardless as it eventually hits net/http where it panics.

igm commented 6 years ago

this was fixed already https://github.com/igm/sockjs-go/commit/23545cb0a62767510433d8acb0d4a6ed1d3670dd are you sure you are using latest master? as per you stack trace line 57 is reported, but in httpreceiver.go it is currently a bracket: https://github.com/igm/sockjs-go/blob/master/sockjs/httpreceiver.go#L57

wavded commented 6 years ago

hmm i'll double check, thx for your quick response

igm commented 6 years ago

writing again regarding the line number, line 46 is the important one (not 57). But as this commit shows https://github.com/igm/sockjs-go/commit/23545cb0a62767510433d8acb0d4a6ed1d3670dd there was indeed a problem on line 46 before the fix.

wavded commented 6 years ago

@igm thanks for the response, I pulled latest master, and I do see the change. I will run that fix and see if things clear up. Thanks.

rjeczalik commented 6 years ago

@wavded @igm I think the real, indirect problem is old git tag - latest tag https://github.com/igm/sockjs-go/tree/v2.0.0 heads at really old commit. When you use go dep tool to vendor sockjs-go it fetches that tag by default and not the master.

igm commented 6 years ago

the same fix is also applied to v2 branch and v2.0.0 tag https://github.com/igm/sockjs-go/blob/v2.0.0/sockjs/httpreceiver.go#L44

But regarding versioning as you brought this topic up, would it make sense to start tagging master branch with v3.y.z starting with v3.0.0? Any feedback is appreciated please, I'm not sure about all the consequences with all the tools: godep, dep, glide, govendor, etc...