fasthttp / websocket

WebSocket implementation for fasthttp.
BSD 2-Clause "Simplified" License
540 stars 55 forks source link

panic in WriteMessage #6

Closed MasterDimmy closed 5 years ago

MasterDimmy commented 5 years ago

PANIC: runtime error: invalid memory address or nil pointer dereference

STACK: goroutine 83 [running]: c:/go/src/runtime/panic.go:513 +0x1c7 github.com/valyala/fasthttp.(*hijackConn).SetWriteDeadline(0xc000004040, 0x0, 0x0, 0x0, 0x46e47b8, 0xc000160101)

:1 +0x39 github.com/fasthttp/websocket.(*Conn).write(0xc00008a000, 0x1, 0x0, 0x0, 0x0, 0xc00051200a, 0x2e4c, 0xfa004, 0xc0002b4e48, 0x0, ...) C:/gopath/src/github.com/fasthttp/websocket/conn.go:376 +0x102 github.com/fasthttp/websocket.(*messageWriter).flushFrame(0xc000501ed8, 0xc0002b2001, 0xc0002b4e48, 0x0, 0x1b8, 0x0, 0x0) C:/gopath/src/github.com/fasthttp/websocket/conn.go:601 +0x24d github.com/fasthttp/websocket.(*Conn).WriteMessage(0xc00008a000, 0x1, 0xc0002b2000, 0x2e48, 0x3000, 0x0, 0x0) C:/gopath/src/github.com/fasthttp/websocket/conn.go:753 +0x252 main.(*Client).writePump(0xc000148030) C:/sssr/test_socket.go:140 +0x19e created by main.serveWebsocket.func1 C:/sssr/test_socket.go:169 +0x13d code is: ... buf, _ := json.Marshal(msgd) err := c.conn.WriteMessage(websocket.TextMessage, buf)
savsgio commented 5 years ago

It's very probably that internal fasthttp connection is closed, so the websocket connection is closed too c.conn. It's mandatory to keep the handler active all time that connection is alived. For that, putting all code inside a loop, with sync.WaitGroup, etc.

If you share more code, I could help you more.

MasterDimmy commented 5 years ago

Thanx, you are correct. fasthttp already closed connection. That pare of code:

func serveWebsocket(ctx *fasthttp.RequestCtx, hub *TSocketHub) {
    ipc := getRealIP(ctx)
    upgrader.Upgrade(ctx, func(conn *websocket.Conn) {
        client := &SocketClient{Hub: hub, Conn: conn, Send: make(chan []byte, 256), ip: ipc}
        client.Hub.register <- client
        client.Send <- sendAllHistoryData()
        go client.writePump()    <-----  goroutine, i think it kept connection
    })
}
//but closed it here

Its wrong way to keep handler active all the time because it uses huge amount of memory on every connection because every function has its agruments on stask.

func serveWebsocket(ctx *fasthttp.RequestCtx, hub *TSocketHub) {
    ipc := getRealIP(ctx)
    upgrader.Upgrade(ctx, func(conn *websocket.Conn) {
        client := &SocketClient{Hub: hub, Conn: conn, Send: make(chan []byte, 256), ip: ipc}
        client.Hub.register <- client
        client.Send <- sendAllHistoryData()
        client.writePump()    <----- here loop to broadcast data every 10 seconds, i removed goroutine, it works, but eats huge amount of memory
    })
}

If i have 10 000 connections, this cause huge memory problems, but it original gorillas \ websocket - doesnt. Compare with this:

func listenServerSocket(w http.ResponseWriter, r *http.Request) {
    defer handlePanic()
    accessp(r, "%s\n", r.URL.String())

    conn, err := upgrader.Upgrade(w, r, nil)
    if err != nil {
        debugpf("chat upgrade error %s", err.Error())
        return
    }

    isadmin := auth_storage.checkLogin(r)

    ip := getRemoteIP(r)

    client := &ChatClient{ip: ip, isadmin: isadmin, conn: conn, send: make(chan *TSocketChatServerMessage, 10), closeconn: make(chan bool)}

    chatSocketHub.register <- client

    // Allow collection of memory referenced by the caller by doing all work in
    // new goroutines.
    go client.writePump()
    go client.readPump()
}

Here connection is not closed after handler exit, so it tooks only *conn on every client, and its not allocated on stack. So in this way i can server more then 1 000 000 of connections in second without huge memory loss.

I think in fasthttp\websocket it's correct to do the same way, not closing connection if *conn is used, as in gorilla is done.

Cant show all code, sorry.

savsgio commented 5 years ago

I think so it's a possible bug. I will check it in a few days.

savsgio commented 5 years ago

I checked fasthttp, don't worry by huge memory problems, because the hijacked connections are proccesed into gorutines by fasthttp. So your fasthttp.RequestHandler is not active all time websocket connection is opened, only upgrade the connection with your websocket.FastHTTPHandler, that it will be processed by Hijack connection.

The difference with net/http is gorilla returns the hijacked connection and you could processed into gorutines or not. Fasthttp does that automatically and always in gorutines.

Just mandatory to keep websocket.FastHTTPHandler active with a loop or something similar.

That's the reason why you pass the websocket.FastHTTPHandler to the upgrader.Upgrade(...)