1
0
mirror of https://github.com/jhillyerd/inbucket.git synced 2026-10-29 05:32:31 +00:00
Files
go-inbucket/pkg/rest/socket_controller.go
James Hillyerd 94472ab496 fix(rest): make WebSocket monitor listener teardown safe under concurrency (#623)
* fix(rest): make WebSocket monitor listener teardown safe under concurrency

The commit-1 tests and race detector exposed three related defects in the
monitor listener's Close, whose check-then-act select was unsound:

1. With a full event queue (a slow or disconnecting client), the
   `case <-ml.c:` branch received a buffered event, mistook it for the
   closed-channel signal, and skipped both RemoveListener and close: the
   listener stayed registered, and the hub actor then blocked forever
   inside Receive on the next dispatch, wedging every hub consumer.
2. The read and write pumps both call Close when the connection drops;
   two callers could pass the select's default branch together and the
   second close panicked with "close of closed channel".
3. Even made exactly-once, closing the event queue was still unsafe:
   RemoveListener returns once its op is queued, not processed, so the
   hub actor could be mid-send into the queue when Close closed it
   (send on closed channel panic inside the actor).

Fix: Close deregisters and closes a `done` channel via sync.Once; the
event queue is never closed.  Receive and Delete select their enqueue
against done and return errListenerClosed when aborted, so the hub
removes the listener itself even if RemoveListener has not run yet, and
a blocked enqueue aborts instead of wedging the actor.  The write pump
exits on done and sends the closing frame.

Each defect has a regression test that fails against the previous code
(verified: no-op close delivered post-close events, concurrent closes
panicked, and close raced the actor's blocked send).  The write pump
keeps its c-close branch for safety; the black-box monitor tests from
commit 1 pass unchanged.

* fix(rest): close listener done channel before enqueueing removal

Copilot review on PR #623 correctly identified a remaining deadlock in
msgListener.Close: when the hub actor is blocked inside this listener's
Receive (full event queue) and the hub op queue is also full,
RemoveListener cannot enqueue, so execution never reached close(ml.done) --
the only operation able to release the actor.  Under the same slow-client
condition, concurrent dispatchers keeping the hub queue saturated made
Close hang permanently.

Fix: close the done channel (guarded by the existing sync.Once) before
queueing the RemoveListener op.  The blocked actor is released first, keeps
draining the op queue, and the removal can then be queued even when the
queue is full.  As a side effect, a Close racing a not-yet-processed
AddListener now also deregisters correctly, since the removal op can no
longer be overtaken by the earlier registration op.

Regression test TestMsgListenerCloseFullHubOpQueue wedges the actor in
Receive with a saturated op queue before calling Close; verified it fails
("Close deadlocked") against the previous ordering and passes with -race
after the fix, along with the full pkg/rest and pkg/msghub suites.
2026-09-20 11:57:04 -07:00

255 lines
8.2 KiB
Go

package rest
import (
"errors"
"net/http"
"sync"
"time"
"github.com/gorilla/websocket"
"github.com/inbucket/inbucket/v3/pkg/extension/event"
"github.com/inbucket/inbucket/v3/pkg/msghub"
"github.com/inbucket/inbucket/v3/pkg/rest/model"
"github.com/inbucket/inbucket/v3/pkg/server/web"
"github.com/inbucket/inbucket/v3/pkg/stringutil"
"github.com/rs/zerolog/log"
)
const (
// socketWriteWait is the time allowed to write a message to the peer.
socketWriteWait = 10 * time.Second
// socketPingPeriod is how often pings are sent to the peer. Must be less than
// socketPongWait.
socketPingPeriod = (socketPongWait * 9) / 10
// socketPongWait is the time allowed to read the next pong message from the peer.
socketPongWait = 60 * time.Second
// socketMaxMessageSize is the maximum message size allowed from the peer.
socketMaxMessageSize = 512
// socketChanLen is the event queue length of a monitor listener.
socketChanLen = 100
)
// upgrader holds the options for monitor WebSocket connection upgrades.
var upgrader = websocket.Upgrader{
ReadBufferSize: 1024,
WriteBufferSize: 1024,
}
// msgListener handles messages from the msghub for a single monitor connection. Stored and
// deleted events are shaped into the API-specific payload by toStored/toDeleted; a nil
// toDeleted drops deletions (socketv1 API behavior).
type msgListener[T any] struct {
hub *msghub.Hub
c chan T
done chan struct{} // Closed by Close; stops event delivery and the write pump
mailbox string // Name of mailbox to monitor, "" == all mailboxes
toStored func(msg event.MessageMetadata) T
toDeleted func(mailbox string, id string) T
closeOnce sync.Once
}
// newMsgListener creates a listener and registers it with the hub. The optional mailbox
// parameter restricts events relayed to the client to that mailbox only.
func newMsgListener[T any](
hub *msghub.Hub,
mailbox string,
toStored func(msg event.MessageMetadata) T,
toDeleted func(mailbox string, id string) T,
) *msgListener[T] {
ml := &msgListener[T]{
hub: hub,
c: make(chan T, socketChanLen),
done: make(chan struct{}),
mailbox: mailbox,
toStored: toStored,
toDeleted: toDeleted,
}
hub.AddListener(ml)
return ml
}
// watches reports whether events for the given mailbox should be relayed to the client.
func (ml *msgListener[T]) watches(mailbox string) bool {
return ml.mailbox == "" || ml.mailbox == mailbox
}
// errListenerClosed is returned by Receive/Delete after Close; the hub removes listeners
// that return errors, providing deregistration even if RemoveListener has not run yet.
var errListenerClosed = errors.New("monitor listener closed")
// Receive handles an incoming message. If the listener is closed while its event queue is
// full, the enqueue is abandoned rather than blocking the hub actor forever.
func (ml *msgListener[T]) Receive(msg event.MessageMetadata) error {
if !ml.watches(msg.Mailbox) {
return nil
}
select {
case ml.c <- ml.toStored(msg):
return nil
case <-ml.done:
return errListenerClosed
}
}
// Delete handles a deleted message.
func (ml *msgListener[T]) Delete(mailbox string, id string) error {
if ml.toDeleted == nil || !ml.watches(mailbox) {
return nil
}
select {
case ml.c <- ml.toDeleted(mailbox, id):
return nil
case <-ml.done:
return errListenerClosed
}
}
// Close removes the listener registration and stops event delivery. Safe for concurrent
// use: the read and write pumps both invoke it when the connection drops. The done channel
// is closed before the removal is queued: a hub actor blocked in this listener's Receive or
// Delete is released first, so it keeps draining the hub op queue and the removal can be
// queued even when that queue is full. The event queue is not closed: RemoveListener
// returns once the op is queued, not processed, so a concurrent hub-actor send into the
// queue could otherwise panic on the close; the done channel ends delivery and unblocks
// any such send.
func (ml *msgListener[T]) Close() {
ml.closeOnce.Do(func() {
// Release a hub actor blocked in Receive or Delete before queueing the removal:
// with the removal queued first, a full hub op queue would block it here forever,
// never reaching the close below -- the only operation able to release the actor.
close(ml.done)
ml.hub.RemoveListener(ml)
})
}
// socketReadPump makes sure the websocket client is still connected, discarding any messages
// the client sends. It returns once the connection fails or the peer closes it; closeFn is
// then invoked to tear the listener down.
func socketReadPump(conn *websocket.Conn, closeFn func()) {
slog := log.With().Str("module", "rest").Str("proto", "WebSocket").
Str("remote", conn.RemoteAddr().String()).Logger()
defer closeFn()
conn.SetReadLimit(socketMaxMessageSize)
if err := conn.SetReadDeadline(time.Now().Add(socketPongWait)); err != nil {
slog.Warn().Err(err).Msg("Failed to setup read deadline")
}
conn.SetPongHandler(func(string) error {
slog.Debug().Msg("Got pong")
if err := conn.SetReadDeadline(time.Now().Add(socketPongWait)); err != nil {
slog.Warn().Err(err).Msg("Failed to set read deadline in pong")
}
return nil
})
for {
if _, _, err := conn.ReadMessage(); err != nil {
if websocket.IsUnexpectedCloseError(
err,
websocket.CloseNormalClosure,
websocket.CloseGoingAway,
websocket.CloseNoStatusReceived,
) {
// Unexpected close code
slog.Warn().Err(err).Msg("Socket error")
} else {
slog.Debug().Msg("Closing socket")
}
break
}
}
}
// socketWritePump relays events from c to the peer as JSON messages until done is closed,
// sending pings while idle to keep the connection alive. closeFn is invoked on exit.
func socketWritePump[T any](c chan T, done <-chan struct{}, conn *websocket.Conn, closeFn func()) {
slog := log.With().Str("module", "rest").Str("proto", "WebSocket").
Str("remote", conn.RemoteAddr().String()).Logger()
ticker := time.NewTicker(socketPingPeriod)
defer func() {
ticker.Stop()
closeFn()
}()
// Handle messages from hub until listener is closed
for {
select {
case event, ok := <-c:
if err := conn.SetWriteDeadline(time.Now().Add(socketWriteWait)); err != nil {
slog.Warn().Err(err).Msg("Failed to set write deadline for msg")
}
if !ok {
// Listener closed, exit
_ = conn.WriteMessage(websocket.CloseMessage, []byte{})
return
}
if conn.WriteJSON(event) != nil {
// Write failed
return
}
case <-done:
// Listener closed, exit
_ = conn.WriteMessage(websocket.CloseMessage, []byte{})
return
case <-ticker.C:
// Send ping
if err := conn.SetWriteDeadline(time.Now().Add(socketWriteWait)); err != nil {
slog.Warn().Err(err).Msg("Failed to set write deadline for ping")
}
if conn.WriteMessage(websocket.PingMessage, []byte{}) != nil {
// Write error
return
}
slog.Debug().Msg("Sent ping")
}
}
}
// serveMonitor upgrades the connection to a WebSocket, registers a new listener with the hub,
// and relays events to the client until it disconnects.
func serveMonitor[T any](
w http.ResponseWriter,
req *http.Request,
hub *msghub.Hub,
mailbox string,
newListener func(hub *msghub.Hub, mailbox string) *msgListener[T],
) error {
// Upgrade to Websocket.
conn, err := upgrader.Upgrade(w, req, nil)
if err != nil {
return err
}
web.ExpWebSocketConnectsCurrent.Add(1)
defer func() {
_ = conn.Close()
web.ExpWebSocketConnectsCurrent.Add(-1)
}()
log.Debug().Str("module", "rest").Str("proto", "WebSocket").
Str("remote", conn.RemoteAddr().String()).Msg("Upgraded to WebSocket")
// Create, register listener; then interact with conn.
ml := newListener(hub, mailbox)
go socketWritePump(ml.c, ml.done, conn, ml.Close)
socketReadPump(conn, ml.Close)
return nil
}
// metadataToHeader converts hub event metadata into a JSON message header.
func metadataToHeader(msg *event.MessageMetadata) *model.JSONMessageHeaderV1 {
return &model.JSONMessageHeaderV1{
Mailbox: msg.Mailbox,
ID: msg.ID,
From: stringutil.StringAddress(msg.From),
To: stringutil.StringAddressList(msg.To),
Subject: msg.Subject,
Date: msg.Date,
PosixMillis: msg.Date.UnixNano() / 1000000,
Size: msg.Size,
}
}