lesismal / go-websocket-benchmark

121 stars 14 forks source link

all: add framework lib/websocket #12

Closed shuLhan closed 1 year ago

shuLhan commented 1 year ago

The lib/websocket [1] is one of the Go WebSocket library that use Linux epoll or BSD kqueue.

[1] https://pkg.go.dev/github.com/shuLhan/share/lib/websocket

lesismal commented 1 year ago

Thanks for PR!

I just read the code of lib/websocket, and found that it should have the same problem as gobwas+poller, because it doesn't implement async parser, but still provide Blocking reading-related interface{}, if some connections a slow, it will block here in the for-loop, and cause other connections waiting for long: https://github.com/shuLhan/share/blob/master/lib/websocket/server.go#L266

So, I think it's not suitable to use it in production.

For more details: https://github.com/gobwas/ws/issues/143

Sorry to say that I will revert this PR.

shuLhan commented 1 year ago

5 Jun 2023 07:52:57 lesismal @.***>:

Thanks for PR!

I just read the code of lib/websocket[https://pkg.go.dev/github.com/shuLhan/share/lib/websocket], and found that it should have the same problem as gobwas+poller, because it doesn't implement async parser, but still provide Blocking reading-related interface{}, if some connections a slow, it will block here in the for-loop, and cause other connections waiting for long:

https://github.com/shuLhan/share/blob/master/lib/websocket/server.go#L266

So, I think it's not suitable to use it in production.

For more details: gobwas/ws#143[https://github.com/gobwas/ws/issues/143]

Sorry to say that I will revert this PR.

— Reply to this email directly, view it on GitHub[https://github.com/lesismal/go-websocket-benchmark/pull/12#issuecomment-1575886176], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AAAKSKULCS3DRRQW4YLSQNLXJUUWNANCNFSM6AAAAAAY2EDGRA]. You are receiving this because you authored the thread.[Tracking image][https://github.com/notifications/beacon/AAAKSKUOVJV2MLG2IJAF2TTXJUUWNA5CNFSM6AAAAAAY2EDGRCWGG33NNVSW45C7OR4XAZNMJFZXG5LFINXW23LFNZ2KUY3PNVWWK3TUL5UWJTS55YOWA.gif]

Hi, thanks for reviewing.

That is interesting find, I will take a look at it.

Will you accept the PR again once I found a way to fix it?

lesismal commented 1 year ago

Will you accept the PR again once I found a way to fix it?

Yes, of course! PR will always be welcomed!

shuLhan commented 1 year ago

@lesismal

After reading several of your comments on gobwas/ws issues 143, gobwas/ws-examples issues 18, and then re-read my code, and test with your code at gobwas/ws-examples/issues/18#issue-867317941, I can say that the lib/websocket does not have blocking issue.

Epool in lib/websocket set the socket to nonblock for each accepted socket [1]. If a client send partial HTTP upgrade, for example one byte at atime, server will reject it immediately [2]. This is by design on HTTP level.

For example, given slow client (SC) and server (S), assume that SC try to send upgrade request "GET / HTTP/1.1" but send only one byte "G"

SC> G S< G S> HTTP/1.1 400 Bad Request\r\n... S> CLOSE

In case client connection has been upgraded and client send partial websocket frame its already handled here [3]. Since the socket is non-block all partial packet received by server will be read and stored until one frame is completed.

So, I think it's not suitable to use it in production.

It's already battle tested in production.

If you can show the code that the lib/websocket has blocking issue, I would be happy to fix it.

[1] https://github.com/shuLhan/share/blob/2d57327dbe2d2d2d819d699f543daac9a55e48e9/lib/net/poll_linux.go#L47 [2] https://github.com/shuLhan/share/blob/2d57327dbe2d2d2d819d699f543daac9a55e48e9/lib/websocket/handshake.go#L149 [3] https://github.com/shuLhan/share/blob/2d57327dbe2d2d2d819d699f543daac9a55e48e9/lib/websocket/frame.go#L330

lesismal commented 1 year ago

@shuLhan lib/websocket's problem is even worse than gobwas/ws, it's not the blocking problem, but easy to failed when parsing streaming data.

package main

import (
    "log"
    "net"
    "time"

    "github.com/shuLhan/share/lib/websocket"
)

var (
    srv *websocket.Server

    addr      = "127.0.0.1:8080"
    handshake = []byte("GET /ws HTTP/1.1\r\n" +
        "Host: 127.0.0.1:8080\r\n" +
        "User-Agent: Go-http-client/1.1\r\n" +
        "Connection: Upgrade\r\n" +
        "Sec-Websocket-Key: w9CBg2aFf0pZFILulHE1Ww==\r\n" +
        "Sec-Websocket-Version: 13\r\n" +
        "Upgrade: websocket\r\n\r\n")
)

func handleText(conn int, payload []byte) {
    packet := websocket.NewFrameText(false, payload)

    err := websocket.Send(conn, packet)
    if err != nil {
        log.Println("handleText: " + err.Error())
    }
    log.Println("onMessage: ", string(payload))
}

var chWait = make(chan int)

func main() {
    time.AfterFunc(time.Second, func() {
        client(0)
        client(time.Second / 100)
    })

    opts := &websocket.ServerOptions{
        Address: addr,
        // HandleAuth:  handleAuth,
        ConnectPath: `/ws`,
        HandleText:  handleText,
    }
    srv := websocket.NewServer(opts)
    srv.Start()
}

func client(sleepTime time.Duration) bool {
    conn, err := net.Dial("tcp", addr)
    if err != nil {
        log.Fatalf("dial failed: %v", err)
    }

    // normal client, send full packet and if lucky the full packet arrive at the same unix.Read
    if sleepTime == 0 {
        _, err := conn.Write(handshake)
        if err != nil {
            log.Fatalf("write handshake failed: %v", err)
        }

    } else {
        // slow client, seperate the full packet to multi packets;
        // you will get err when conn.Read below.
        // in public internet, it's normal that the tcp-streaming data comes not in the same time,
        // or maybe just not in the same unix.Read, that's so easy to result to an error when your
        // parser assume the full-packet data comes  1 by 1 and everytime your unix.Read get a full packet exactly.
        _, err := conn.Write(handshake[:5])
        if err != nil {
            log.Fatalf("write half handshake failed: %v", err)
        }
        close(chWait)
        time.Sleep(sleepTime)

        _, err = conn.Write(handshake[5:])
        if err != nil {
            log.Fatalf("write half handshake failed: %v", err)
        }
    }

    buf := make([]byte, 2048)
    n, err := conn.Read(buf)
    if err != nil {
        log.Fatalf("read handshake failed: %v", err)
    }
    log.Printf("response: \n%v", string(buf[:n]))

    return n > 0
}

output:

root@ubuntu:~/slow# go run test.go 
2023/06/05 17:34:14 response: 
HTTP/1.1 101 Switching Protocols
Upgrade: websocket
Connection: Upgrade
Sec-Websocket-Accept: 29EdDSe9X5uqHkhTQj6jglN2hok=

2023/06/05 17:34:14 response: 
HTTP/1.1 400 invalid HTTP pragma: : bad request  ## look at here, when unix.Read doesn't get the full packet, just make it failed then the connection can not go on
shuLhan commented 1 year ago

// in public internet, it's normal that the tcp-streaming data comes not in the same time, // or maybe just not in the same unix.Read, that's so easy to result to an error when your // parser assume the full-packet data comes 1 by 1 and everytime your unix.Read get a full packet exactly

Yes it is normal, but this part should already handled in OS level, by buffering packet before sending or receiving.

HTTP/1.1 400 invalid HTTP pragma: : bad request ## look at here, when unix.Read doesn't get the full packet, just make it failed then the connection can not go on

Yes it is by design. A minimum WebSocket handshake is 128 bytes. A TCP payload in single packet can handle at least 1460 bytes. If the client cannot send full handshake in single write then it is not worth to pursue, otherwise the server will open to slow attack, a.k.a slowloris.

I think you are mixing HTTP and WebSocket. Unlike HTTP, WebSocket is full-duplex, both server and client try to keep the connection persistent as long as possible. Maintaining clients that have persistent, stable connections is preferred over handling client that cannot even send full handshake.

lesismal commented 1 year ago

Yes it is by design. A minimum WebSocket handshake is 128 bytes. A TCP payload in single packet can handle at least 1460 bytes.

the TCP protocol can't promise the full handshake arrives in 1 Read.

lesismal commented 1 year ago

And, see another example, your socket fd is not nonblocking, so after Accept, if the socket doesn't send even 1 byte, your Recv here will block: https://github.com/shuLhan/share/blob/master/lib/websocket/server.go#L266

package main

import (
    "log"
    "net"
    "net/url"
    "time"

    gorillaWS "github.com/gorilla/websocket"
    "github.com/shuLhan/share/lib/websocket"
)

var (
    srv *websocket.Server

    addr      = "127.0.0.1:8080"
    handshake = []byte("GET /ws HTTP/1.1\r\n" +
        "Host: 127.0.0.1:8080\r\n" +
        "User-Agent: Go-http-client/1.1\r\n" +
        "Connection: Upgrade\r\n" +
        "Sec-Websocket-Key: w9CBg2aFf0pZFILulHE1Ww==\r\n" +
        "Sec-Websocket-Version: 13\r\n" +
        "Upgrade: websocket\r\n\r\n")
)

func handleText(conn int, payload []byte) {
    packet := websocket.NewFrameText(false, payload)

    err := websocket.Send(conn, packet)
    if err != nil {
        log.Println("handleText: " + err.Error())
    }
    log.Println("onMessage: ", string(payload))
}

var chWaitDial = make(chan int)

func main() {
    time.AfterFunc(time.Second, func() {
        go client(time.Second * 30)

        // step 2: waiting for the first client dial success
        //         then start to dial with timeout
        <-chWaitDial
        u := url.URL{Scheme: "ws", Host: addr, Path: "/ws"}
        gorillaWS.DefaultDialer.HandshakeTimeout = time.Second
        conn, _, err := gorillaWS.DefaultDialer.Dial(u.String(), nil)
        if err != nil {
            // will get error of timeout here
            log.Fatalf("dial failed: %v", err)
        }
        log.Printf("gorillaWS dial: %v", err)
        conn.WriteMessage(gorillaWS.TextMessage, []byte("hello"))
    })

    opts := &websocket.ServerOptions{
        Address:     addr,
        ConnectPath: `/ws`,
        HandleText:  handleText,
    }
    srv := websocket.NewServer(opts)
    srv.Start()
}

func client(sleepTime time.Duration) bool {
    conn, err := net.Dial("tcp", addr)
    if err != nil {
        log.Fatalf("dial failed: %v", err)
    }
    // step 1: after dialing success, and before sending handshake, this client sleep for a while
    //         but allow the normal client to dial
    close(chWaitDial)

    if sleepTime > 0 {
        time.Sleep(sleepTime)
    }

    _, err = conn.Write(handshake)
    if err != nil {
        log.Fatalf("write handshake failed: %v", err)
    }

    buf := make([]byte, 2048)
    n, err := conn.Read(buf)
    if err != nil {
        log.Fatalf("read handshake failed: %v", err)
    }
    log.Printf("response: \n%v", string(buf[:n]))

    return n > 0
}

output:

root@ubuntu:~/slow# go run ./test.go 
2023/06/06 01:37:07 dial failed: read tcp 127.0.0.1:33196->127.0.0.1:8080: i/o timeout
exit status 1