igm / sockjs-go

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

sockjs: do not close session server-side #65

Closed rjeczalik closed 6 years ago

rjeczalik commented 6 years ago

Fixes the following error in BenchmarkMessageWebsocket:

 WriteMessage()=write tcp x->y: write: broken pipe

This change is Reviewable

coveralls commented 6 years ago

Coverage Status

Coverage decreased (-0.1%) to 94.911% when pulling 41a1039f3f80cc34653728196e2bd5f015641d65 on rjeczalik:bench-fix into 8b4d9e0d4f886816ea7df034c1aad5173dc6ae96 on igm:master.

FZambia commented 6 years ago

An alternative way of fix:

func BenchmarkMessageWebsocket(b *testing.B) {
    flag.Parse()

    msg := strings.Repeat("x", *size)
    wsFrame := []byte(fmt.Sprintf("[%q]", msg))

    opts := Options{
        Websocket:       true,
        SockJSURL:       "//cdnjs.cloudflare.com/ajax/libs/sockjs-client/0.3.4/sockjs.min.js",
        HeartbeatDelay:  time.Hour,
        DisconnectDelay: time.Hour,
        ResponseLimit:   uint32(*size),
    }

    h := NewHandler("/echo", opts, func(session Session) {
        for {
            msg, err := session.Recv()
            if err != nil {
                if session.GetSessionState() != SessionActive {
                    break
                }
                b.Fatalf("Recv()=%s", err)
            }

            if err := session.Send(msg); err != nil {
                b.Fatalf("Send()=%s", err)
            }
        }
    })

    server := httptest.NewServer(h)
    defer server.Close()

    clients := make([]*websocket.Conn, *clients)

    for i := range clients {
        url := "ws" + server.URL[4:] + fmt.Sprintf("/echo/server/%d/websocket", i)

        client, _, err := websocket.DefaultDialer.Dial(url, nil)
        if err != nil {
            b.Fatalf("%d: Dial()=%s", i, err)
        }

        _, p, err := client.ReadMessage()
        if err != nil || string(p) != "o" {
            b.Fatalf("%d: failed to start new session: frame=%v, err=%v", i, p, err)
        }

        clients[i] = client
    }

    var wg sync.WaitGroup

    b.ReportAllocs()
    b.ResetTimer()

    for _, c := range clients {
        wg.Add(1)

        go func(client *websocket.Conn) {
            defer wg.Done()
            defer client.Close()

            for i := 0; i < b.N; i++ {
                if err := client.WriteMessage(websocket.TextMessage, wsFrame); err != nil {
                    b.Fatalf("WriteMessage()=%s", err)
                }

                _, _, err := client.ReadMessage()
                if err != nil {
                    b.Fatalf("ReadMessage()=%s", err)
                }
            }
        }(c)
    }

    wg.Wait()
}

Not really sure which is cleaner:)

FZambia commented 6 years ago

Btw - is there a lot of sense having many concurrent clients by default? Looks like this benchmark passes messages between clients independently - so run it with one concurrent client only could give the same result.

rjeczalik commented 6 years ago

An alternative way of fix:

@FZambia Thanks for the suggestion, this removes the need for the done frame - I've settled with your version.

(...) is there a lot of sense having many concurrent clients by default?

I was also using this benchmark to test how the code I've written worked in concurrent scenario. I agree, for the benchmark purpose it's not needed. I'm not sure you're suggesting we should just change the default or do not run multiple clients at all - I assumed the latter. :)

igm commented 6 years ago

I agree with using just one client for this benchmark and thanks for fixing it

:lgtm:

Reviewed 1 of 1 files at r2. Review status: :shipit: complete! all files reviewed, all discussions resolved


Comments from Reviewable