delthas: 1 Add support for the upstream echo-message capability 3 files changed, 103 insertions(+), 92 deletions(-)
> I've thought about this a bit more, and I'd really prefer not turning > on echo-message upstream if it doesn't support labeled-response. This > special-case increases the cognitive load quite a bit and makes things > less predictable, harder to debug. FWIW, should be easy by sending > `CAP REQ :echo-message labeled-response` in upstreamConn.requestCaps, > and revert the extra requestCaps call sites. > > Additionally, that patch would be easier to review if broken down into > smaller pieces. For instance, one patch to handle `CAP ACK -cap` This one can be dropped if we don't turn on echo-message depending on downstream connections capabilities. > one to merge PRIVMSG/NOTICE with TAGMSG Done here: https://git.sr.ht/~emersion/soju/commit/9513c28208e9f93262fa57ae43d4f247dbece38a
Copy & paste the following snippet into your terminal to import this patchset into git:
curl -s https://lists.sr.ht/~emersion/soju-dev/patches/30651/mbox | git am -3Learn more about email & git
This adds support for upstream echo-message. This capability is enabled: - either if the upstream supports labeled-response, in which case we can prevent echo messages from being routed to the origin downstream if it did not request it (because we have its ID in the message tags), - or if all downstreams support echo-message, in which case we do not need to filter echo messages out. The former is the general case, as most servers that support echo-message also support labeled-response, and the latter is a fallback. When it is enabled, we don't echo downstream messages in the downstream handler, but rather wait for the upstream to echo it, to produce it to downstreams. When it is disabled, we keep the same behaviour as before: produce the message to all downstreams as soon as it is received from the downstream. In other words, the main functional difference is that when all upstreams support echo-message, of if the upstream supports labeled-response, the client will now receive an echo for its messages when the server acknowledges them, rather than when soju acks them. Additionally, the downstream PRIVMSG/NOTICE/TAGMSG handler was slightly refactored into a common switch case as there was starting to be a lot of common code. uc.produce was also refactored to take an ID rather than a downstream. --- downstream.go | 137 ++++++++++++++++++++++---------------------------- upstream.go | 56 +++++++++++++++------ user.go | 2 + 3 files changed, 103 insertions(+), 92 deletions(-) diff --git a/downstream.go b/downstream.go index 49f6807..bf40fc7 100644 --- a/downstream.go +++ b/downstream.go @@ -917,6 +917,10 @@ func (dc *downstreamConn) handleCapCommand(cmd string, args []string) error { if !dc.registered { dc.registration.negotiatingCaps = true + } else { + dc.forEachUpstream(func(uc *upstreamConn) { + uc.updateCaps() + }) } case "END": if !dc.registered { @@ -2416,15 +2420,25 @@ func (dc *downstreamConn) handleMessageRegistered(ctx context.Context, msg *irc. Command: "WHOIS", Params: params, }) - case "PRIVMSG", "NOTICE": + case "PRIVMSG", "NOTICE", "TAGMSG": + tag := msg.Command == "TAGMSG" var targetsStr, text string - if err := parseMessageParams(msg, &targetsStr, &text); err != nil { + if err := parseMessageParams(msg, &targetsStr); err != nil { return err } + if !tag { + if err := parseMessageParams(msg, nil, &text); err != nil { + return err + } + } tags := copyClientTags(msg.Tags) for _, name := range strings.Split(targetsStr, ",") { - if name == "$"+dc.srv.Config().Hostname || (name == "$*" && dc.network == nil) { + msgParams := []string{name} + if !tag { + msgParams = append(msgParams, text) + } + if !tag && (name == "$"+dc.srv.Config().Hostname || (name == "$*" && dc.network == nil)) { // "$" means a server mask follows. If it's the bouncer's // hostname, broadcast the message to all bouncer users. if !dc.user.Admin { @@ -2443,7 +2457,7 @@ func (dc *downstreamConn) handleMessageRegistered(ctx context.Context, msg *irc. Tags: broadcastTags, Prefix: servicePrefix, Command: msg.Command, - Params: []string{name, text}, + Params: msgParams, } dc.srv.forEachUser(func(u *user) { u.events <- eventBroadcast{broadcastMsg} @@ -2456,12 +2470,12 @@ func (dc *downstreamConn) handleMessageRegistered(ctx context.Context, msg *irc. Tags: msg.Tags.Copy(), Prefix: dc.prefix(), Command: msg.Command, - Params: []string{name, text}, + Params: msgParams, }) continue } - if msg.Command == "PRIVMSG" && casemapASCII(name) == serviceNickCM { + if msg.Command != "NOTICE" && casemapASCII(name) == serviceNickCM { if dc.caps.IsEnabled("echo-message") { echoTags := tags.Copy() echoTags["time"] = irc.TagValue(formatServerTime(time.Now())) @@ -2469,10 +2483,12 @@ func (dc *downstreamConn) handleMessageRegistered(ctx context.Context, msg *irc. Tags: echoTags, Prefix: dc.prefix(), Command: msg.Command, - Params: []string{name, text}, + Params: msgParams, }) } - handleServicePRIVMSG(ctx, dc, text) + if !tag { + handleServicePRIVMSG(ctx, dc, text) + } continue } @@ -2480,84 +2496,51 @@ func (dc *downstreamConn) handleMessageRegistered(ctx context.Context, msg *irc. if err != nil { return err } + msgParams[0] = upstreamName - if msg.Command == "PRIVMSG" && uc.network.casemap(upstreamName) == "nickserv" { - dc.handleNickServPRIVMSG(ctx, uc, text) - } - - unmarshaledText := text - if uc.isChannel(upstreamName) { - unmarshaledText = dc.unmarshalText(uc, text) - } - uc.SendMessageLabeled(ctx, dc.id, &irc.Message{ - Tags: tags, - Command: msg.Command, - Params: []string{upstreamName, unmarshaledText}, - }) - - echoTags := tags.Copy() - echoTags["time"] = irc.TagValue(formatServerTime(time.Now())) - if uc.account != "" { - echoTags["account"] = irc.TagValue(uc.account) - } - echoMsg := &irc.Message{ - Tags: echoTags, - Prefix: &irc.Prefix{Name: uc.nick}, - Command: msg.Command, - Params: []string{upstreamName, text}, - } - uc.produce(upstreamName, echoMsg, dc) - - uc.updateChannelAutoDetach(upstreamName) - } - case "TAGMSG": - var targetsStr string - if err := parseMessageParams(msg, &targetsStr); err != nil { - return err - } - tags := copyClientTags(msg.Tags) - - for _, name := range strings.Split(targetsStr, ",") { - if dc.network == nil && casemapASCII(name) == dc.nickCM { - dc.SendMessage(&irc.Message{ - Tags: msg.Tags.Copy(), - Prefix: dc.prefix(), - Command: "TAGMSG", - Params: []string{name}, - }) + if tag && !uc.caps.IsEnabled("message-tags") { continue } - if casemapASCII(name) == serviceNickCM { - continue + if msg.Command == "PRIVMSG" && uc.network.casemap(upstreamName) == "nickserv" { + dc.handleNickServPRIVMSG(ctx, uc, text) } - uc, upstreamName, err := dc.unmarshalEntity(name) - if err != nil { - return err - } - if !uc.caps.IsEnabled("message-tags") { - continue + if !tag { + unmarshaledText := text + if uc.isChannel(upstreamName) { + unmarshaledText = dc.unmarshalText(uc, text) + } + uc.SendMessageLabeled(ctx, dc.id, &irc.Message{ + Tags: tags, + Command: msg.Command, + Params: []string{upstreamName, unmarshaledText}, + }) + } else { + uc.SendMessageLabeled(ctx, dc.id, &irc.Message{ + Tags: tags, + Command: msg.Command, + Params: []string{upstreamName}, + }) } - uc.SendMessageLabeled(ctx, dc.id, &irc.Message{ - Tags: tags, - Command: "TAGMSG", - Params: []string{upstreamName}, - }) - - echoTags := tags.Copy() - echoTags["time"] = irc.TagValue(formatServerTime(time.Now())) - if uc.account != "" { - echoTags["account"] = irc.TagValue(uc.account) - } - echoMsg := &irc.Message{ - Tags: echoTags, - Prefix: &irc.Prefix{Name: uc.nick}, - Command: "TAGMSG", - Params: []string{upstreamName}, + // If the upstream supports echo message, we'll produce the message + // when it is echoed from the upstream. + // Otherwise, produce/log it here because it's the last time we'll see it. + if !uc.caps.IsEnabled("echo-message") { + echoTags := tags.Copy() + echoTags["time"] = irc.TagValue(formatServerTime(time.Now())) + if uc.account != "" { + echoTags["account"] = irc.TagValue(uc.account) + } + echoMsg := &irc.Message{ + Tags: echoTags, + Prefix: &irc.Prefix{Name: uc.nick}, + Command: msg.Command, + Params: msgParams, + } + uc.produce(upstreamName, echoMsg, dc.id) } - uc.produce(upstreamName, echoMsg, dc) uc.updateChannelAutoDetach(upstreamName) } diff --git a/upstream.go b/upstream.go index 56d604e..42d25a7 100644 --- a/upstream.go +++ b/upstream.go @@ -483,15 +483,17 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) err } if msg.Prefix.User == "" && msg.Prefix.Host == "" { // server message - uc.produce("", msg, nil) + uc.produce("", msg, 0) } else { // regular user message target := entity if uc.isOurNick(target) { target = msg.Prefix.Name } + self := uc.isOurNick(msg.Prefix.Name) + ch := uc.network.channels.Value(target) - if ch != nil && msg.Command != "TAGMSG" { + if ch != nil && msg.Command != "TAGMSG" && !self { if ch.Detached { uc.handleDetachedMessage(ctx, ch, msg) } @@ -502,7 +504,7 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) err } } - uc.produce(target, msg, nil) + uc.produce(target, msg, downstreamID) } case "CAP": var subCmd string @@ -525,7 +527,7 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) err break // wait to receive all capabilities } - uc.requestCaps() + uc.updateCaps() if uc.requestSASL() { break // we'll send CAP END after authentication is completed @@ -542,7 +544,14 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) err caps := strings.Fields(subParams[0]) for _, name := range caps { - if err := uc.handleCapAck(ctx, strings.ToLower(name), subCmd == "ACK"); err != nil { + var enable bool + if strings.HasPrefix(name, "-") { + name = strings.TrimPrefix(name, "-") + enable = false + } else { + enable = subCmd == "ACK" + } + if err := uc.handleCapAck(ctx, strings.ToLower(name), enable); err != nil { return err } } @@ -557,7 +566,7 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) err return newNeedMoreParamsError(msg.Command) } uc.handleSupportedCaps(subParams[0]) - uc.requestCaps() + uc.updateCaps() case "DEL": if len(subParams) < 1 { return newNeedMoreParamsError(msg.Command) @@ -1005,7 +1014,7 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) err chMsg := msg.Copy() chMsg.Params[0] = ch - uc.produce(ch, chMsg, nil) + uc.produce(ch, chMsg, 0) } case "PART": var channels string @@ -1031,7 +1040,7 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) err chMsg := msg.Copy() chMsg.Params[0] = ch - uc.produce(ch, chMsg, nil) + uc.produce(ch, chMsg, 0) } case "KICK": var channel, user string @@ -1050,7 +1059,7 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) err ch.Members.Delete(user) } - uc.produce(channel, msg, nil) + uc.produce(channel, msg, 0) case "QUIT": if uc.isOurNick(msg.Prefix.Name) { uc.logger.Printf("quit") @@ -1100,7 +1109,7 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) err } else { ch.Topic = "" } - uc.produce(ch.Name, msg, nil) + uc.produce(ch.Name, msg, 0) case "MODE": var name, modeStr string if err := parseMessageParams(msg, &name, &modeStr); err != nil { @@ -1858,7 +1867,7 @@ func (uc *upstreamConn) handleSupportedCaps(capsStr string) { } } -func (uc *upstreamConn) requestCaps() { +func (uc *upstreamConn) updateCaps() { var requestCaps []string for c := range permanentUpstreamCaps { if uc.caps.IsAvailable(c) && !uc.caps.IsEnabled(c) { @@ -1866,6 +1875,21 @@ func (uc *upstreamConn) requestCaps() { } } + // Enable upstream echo-message if: + // - upstream supports labeled-response, or + // - if every downstream supports echo-message. + echoMessage := true + if !uc.caps.IsAvailable("labeled-response") { + uc.forEachDownstream(func(dc *downstreamConn) { + echoMessage = echoMessage && dc.caps.IsEnabled("echo-message") + }) + } + if !uc.caps.IsEnabled("echo-message") && echoMessage { + requestCaps = append(requestCaps, "echo-message") + } else if uc.caps.IsEnabled("echo-message") && !echoMessage { + requestCaps = append(requestCaps, "-echo-message") + } + if len(requestCaps) == 0 { return } @@ -1931,6 +1955,7 @@ func (uc *upstreamConn) handleCapAck(ctx context.Context, name string, ok bool) Command: "AUTHENTICATE", Params: []string{auth.Mechanism}, }) + case "echo-message": default: if permanentUpstreamCaps[name] { break @@ -2096,9 +2121,10 @@ func (uc *upstreamConn) appendLog(entity string, msg *irc.Message) (msgID string // produce appends a message to the logs and forwards it to connected downstream // connections. // -// If origin is not nil and origin doesn't support echo-message, the message is -// forwarded to all connections except origin. -func (uc *upstreamConn) produce(target string, msg *irc.Message, origin *downstreamConn) { +// originID is the id of the downstream (origin) that sent the message. If it is not 0 +// and origin doesn't support echo-message, the message is forwarded to all +// connections except origin. +func (uc *upstreamConn) produce(target string, msg *irc.Message, originID uint64) { var msgID string if target != "" { msgID = uc.appendLog(target, msg) @@ -2109,7 +2135,7 @@ func (uc *upstreamConn) produce(target string, msg *irc.Message, origin *downstr detached := ch != nil && ch.Detached uc.forEachDownstream(func(dc *downstreamConn) { - if !detached && (dc != origin || dc.caps.IsEnabled("echo-message")) { + if !detached && (dc.id != originID || dc.caps.IsEnabled("echo-message")) { dc.sendMessageWithID(dc.marshalMessage(msg, uc.network), msgID) } else { dc.advanceMessageWithID(msg, msgID) diff --git a/user.go b/user.go index bf1909f..cfa4135 100644 --- a/user.go +++ b/user.go @@ -658,6 +658,7 @@ func (u *user) run() { u.forEachUpstream(func(uc *upstreamConn) { uc.updateAway() + uc.updateCaps() }) case eventDownstreamDisconnected: dc := e.dc @@ -676,6 +677,7 @@ func (u *user) run() { u.forEachUpstream(func(uc *upstreamConn) { uc.cancelPendingCommandsByDownstreamID(dc.id) uc.updateAway() + uc.updateCaps() uc.updateMonitor() }) case eventDownstreamMessage: base-commit: 3d8022d03094381cad1b290936960f152ff36946 -- 2.17.1
I've thought about this a bit more, and I'd really prefer not turning on echo-message upstream if it doesn't support labeled-response. This special-case increases the cognitive load quite a bit and makes things less predictable, harder to debug. FWIW, should be easy by sending `CAP REQ :echo-message labeled-response` in upstreamConn.requestCaps, and revert the extra requestCaps call sites. Additionally, that patch would be easier to review if broken down into smaller pieces. For instance, one patch to handle `CAP ACK -cap`, one to merge PRIVMSG/NOTICE with TAGMSG, and one last patch with the rest.