fix(ircserver): KILL notify-and-close must not block the killer
Some checks failed
check / check (push) Failing after 24s
Some checks failed
check / check (push) Failing after 24s
WIP: Disconnect ran two blocking writes to the victim's socket on the killer's goroutine with the full 30s writeTimeout each, so a victim that stopped reading stalled the killer up to ~60s -- wedging the operator's serve() loop or the HTTP KILL request. Move the notify-and-close to its own goroutine and bound both writes with a short killWriteWindow.
This commit is contained in:
@@ -22,6 +22,7 @@ const (
|
||||
maxLineLen = 512
|
||||
readTimeout = 5 * time.Minute
|
||||
writeTimeout = 30 * time.Second
|
||||
killWriteWindow = 2 * time.Second
|
||||
dnsTimeout = 3 * time.Second
|
||||
pollInterval = 100 * time.Millisecond
|
||||
pingInterval = 90 * time.Second
|
||||
@@ -112,6 +113,18 @@ func newConn(
|
||||
// victim would keep a socket that looks alive but silently
|
||||
// delivers nothing. Disconnect is called from the killer's
|
||||
// goroutine, never the victim's.
|
||||
//
|
||||
// The notify-and-close half runs on its own goroutine and
|
||||
// under a short deadline. There is no per-client send
|
||||
// queue: send() writes straight to the victim's socket, so
|
||||
// a victim that has stopped reading would otherwise stall
|
||||
// the killer for the full writeTimeout on each of the two
|
||||
// writes -- wedging the operator's own serve() loop on the
|
||||
// IRC path, or the API request on the HTTP path. Any
|
||||
// client could trigger that deliberately. Disconnect
|
||||
// therefore returns as soon as the victim is marked closed;
|
||||
// the socket is closed shortly afterwards regardless of
|
||||
// whether the notification could be delivered.
|
||||
func (c *Conn) Disconnect(reason string) {
|
||||
c.mu.Lock()
|
||||
|
||||
@@ -131,19 +144,36 @@ func (c *Conn) Disconnect(reason string) {
|
||||
nick = "*"
|
||||
}
|
||||
|
||||
c.sendFromServer(irc.CmdKill, nick, reason)
|
||||
c.send(
|
||||
"ERROR :Closing Link: " + host +
|
||||
" (" + reason + ")",
|
||||
)
|
||||
|
||||
// Stop the relay goroutine, which would otherwise keep
|
||||
// polling a queue belonging to a deleted session.
|
||||
if c.cancel != nil {
|
||||
c.cancel()
|
||||
}
|
||||
|
||||
c.conn.Close() //nolint:errcheck,gosec
|
||||
go c.notifyKilledAndClose(nick, host, reason)
|
||||
}
|
||||
|
||||
// notifyKilledAndClose delivers the KILL and ERROR lines to
|
||||
// a killed victim and then closes its socket. It runs on a
|
||||
// goroutine owned by neither the killer nor the victim, and
|
||||
// bounds both writes with killWriteWindow so an unresponsive
|
||||
// victim cannot hold the socket open indefinitely.
|
||||
func (c *Conn) notifyKilledAndClose(
|
||||
nick, host, reason string,
|
||||
) {
|
||||
defer c.conn.Close() //nolint:errcheck,gosec
|
||||
|
||||
c.sendWithin(
|
||||
killWriteWindow,
|
||||
FormatMessage(
|
||||
c.serverSfx, irc.CmdKill, nick, reason,
|
||||
),
|
||||
)
|
||||
c.sendWithin(
|
||||
killWriteWindow,
|
||||
"ERROR :Closing Link: "+host+
|
||||
" ("+reason+")",
|
||||
)
|
||||
}
|
||||
|
||||
// buildCommandMap returns a map from IRC command strings
|
||||
@@ -285,11 +315,22 @@ func (c *Conn) currentNick() string {
|
||||
|
||||
// send writes a formatted IRC line to the connection.
|
||||
func (c *Conn) send(line string) {
|
||||
c.sendWithin(writeTimeout, line)
|
||||
}
|
||||
|
||||
// sendWithin writes a formatted IRC line to the connection
|
||||
// under the given write deadline. Callers that must not be
|
||||
// held hostage by an unresponsive peer pass a shorter window
|
||||
// than writeTimeout.
|
||||
func (c *Conn) sendWithin(
|
||||
timeout time.Duration,
|
||||
line string,
|
||||
) {
|
||||
c.writeMu.Lock()
|
||||
defer c.writeMu.Unlock()
|
||||
|
||||
_ = c.conn.SetWriteDeadline(
|
||||
time.Now().Add(writeTimeout),
|
||||
time.Now().Add(timeout),
|
||||
)
|
||||
|
||||
_, _ = fmt.Fprintf(c.conn, "%s\r\n", line)
|
||||
|
||||
Reference in New Issue
Block a user