Conversation
terassyi
commented
Aug 18, 2025
terassyi
commented
- Add websocket server/client
- Add metrics
- Add README
- Add maintenance procedures and image building parts
terassyi
force-pushed
the
add-websocket-keepalive
branch
from
750897e to
2c3bfb2
Compare
| } | ||
| if metricsConfig.Export { | ||
| slog.Info("Start metrics server", "listen", m.AddrPort) | ||
| go func() { |
There was a problem hiding this comment.
It looks like this goroutine never ends.
There was a problem hiding this comment.
It doesn't have to infinitely try to start metrics server, I found.
So, when it fails to start it , I changed to make this panic.
terassyi
force-pushed
the
add-websocket-keepalive
branch
from
2c3bfb2 to
1a9a48f
Compare
|
@tkna |
| go func() { | ||
| if err := m.Metrics.Serve(); err != nil { | ||
| slog.Error("metrics server failed", "error", err) | ||
| // When metrics server doesn't run correctly, program shouldn't do any more. Just panic here. | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Technically, we have 2 web servers (metrics server / websocket server) and we have to shutdown both (in the case of both normal and abnormal closure).
Using errgroup is a good idea for this.
ref: https://github.com/cybozu-private/sphinx/blob/master/cmd/sphinx/main.go
but the implementation here looks like overkill for our case, so it's up to you to use this pattern.
| go func() { | ||
| slog.Info("WebSocket server starting", "addr", server.Addr) | ||
| if err := server.ListenAndServe(); err != nil && err != http.ErrServerClosed { | ||
| slog.Error("Server failed to start", "error", err) |
There was a problem hiding this comment.
ListenAndServe() exits not only at startup, so this message can be a little confusing.
| type conn struct { | ||
| *websocket.Conn | ||
| remoteAddr string | ||
| serverCloseCh chan struct{} |
There was a problem hiding this comment.
[nit] maybe Ch and serverClose are redundant according to Go naming guide. I think a simpler name goes well like interrupt.
https://google.github.io/styleguide/go/decisions#variable-name-vs-type
https://google.github.io/styleguide/go/decisions#external-context-vs-local-names
| defer func() error { | ||
| c.Close() | ||
| return connections.unregister(c.remoteAddr) | ||
| }() |
There was a problem hiding this comment.
I think c.Close() and connections.unregister() should be moved to handleWebSocket() (just after creating/registering)
as error handling in defer is difficult.
https://tech.yappli.io/entry/understanding-defer-in-go
| closeMessage := websocket.FormatCloseMessage(websocket.CloseNormalClosure, "from server") | ||
| if err := c.WriteControl(websocket.CloseMessage, closeMessage, time.Now().Add(5*time.Second)); err != nil { | ||
| slog.Error("failed to write close message", "error", err) | ||
| return nil |
There was a problem hiding this comment.
I think break is better here.
| connections.register(r.RemoteAddr, conn) | ||
|
|
||
| if err := conn.handleWebsocketConnection(context.Background()); err != nil { | ||
| slog.Error("faile to handle websocket connectioin", "remote_addr", r.RemoteAddr) |
There was a problem hiding this comment.
| slog.Error("faile to handle websocket connectioin", "remote_addr", r.RemoteAddr) | |
| slog.Error("failed to handle websocket connection", "remote_addr", r.RemoteAddr) |
| type conn struct { | ||
| *websocket.Conn | ||
| closeCh chan struct{} | ||
| closing atomic.Bool |
|
|
||
| type conn struct { | ||
| *websocket.Conn | ||
| closeCh chan struct{} |
There was a problem hiding this comment.
closeCh doesn't need to be a member of conn.
| } | ||
| u := url.URL{Scheme: "ws", Host: addr.AddrPort().String(), Path: "/ws"} | ||
|
|
||
| // ctx := context.Background() |
| } | ||
| if metricsConfig.Export { | ||
| slog.Info("Start metrics server", "listen", m.AddrPort) | ||
| go func() { |
| } | ||
|
|
||
| func (c *conn) handleWebsocketConnection(ctx context.Context, config *Config, m *Metrics) error { | ||
| msgCh := make(chan message) |
There was a problem hiding this comment.
I think a name like pongReceived or pongMessage is easier to understand.
| return nil | ||
| case <-c.ctrlC: | ||
| slog.Info("Interrupt received, closing connection", "remote_addr", c.RemoteAddr().String()) | ||
| // acitve close sequence. |
There was a problem hiding this comment.
| // acitve close sequence. | |
| // active close sequence. |
| if retryCount >= config.MaxPingRetries { | ||
| // decide the connection is broken. | ||
| slog.Error("Reached to retry limit. The connection is broken.", "remote_addr", c.RemoteAddr().String()) | ||
| c.closeCh <- struct{}{} |
There was a problem hiding this comment.
This is not needed.
(If we want graceful closing we need active close sequence here, but I think force closing is OK in this case)
| m.incrementRetryCount() | ||
| slog.Debug("Sending ping message for retry", "retry", retryCount, "max-retry", config.MaxPingRetries, "remote_addr", c.RemoteAddr().String()) | ||
| } | ||
| slog.Debug("Sending ping message", "remote_addr", c.RemoteAddr()) |
There was a problem hiding this comment.
| slog.Debug("Sending ping message", "remote_addr", c.RemoteAddr()) | |
| slog.Debug("Sending ping message", "remote_addr", c.RemoteAddr().String()) |
| - `established` | ||
| - Websocket で接続が確立していることを示す。確立していたら 1 を出力する。 | ||
| - `ping_retry_count_total` | ||
| - Ping メッセージを再送した回数を示すカウンタ。 |
Signed-off-by: terashima <tomoya-terashima@cybozu.co.jp>
Signed-off-by: terashima <tomoya-terashima@cybozu.co.jp>
Signed-off-by: terashima <tomoya-terashima@cybozu.co.jp>
Signed-off-by: terashima <tomoya-terashima@cybozu.co.jp>
Signed-off-by: terashima <tomoya-terashima@cybozu.co.jp>
Signed-off-by: terashima <tomoya-terashima@cybozu.co.jp>
Signed-off-by: naoki-take <naoki-take@cybozu.co.jp>
terassyi
force-pushed
the
add-websocket-keepalive
branch
from
60715be to
2daede9
Compare
Signed-off-by: terashima <iscale821@gmail.com>
3 participants
