~emersion/soju-dev

Add support for the upstream echo-message capability v2 SUPERSEDED

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
Export patchset (mbox)
How do I use this?

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 -3
Learn more about email & git

[PATCH v2] Add support for the upstream echo-message capability Export this patch

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.