Support METADATA for users #54
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "delthas/soju:fix-metadata-sub"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Also, send METADATA messages on METADATA SUB, so that a client
can subscribe to keys after conn-reg.
We now send METADATA messages:
In order to avoid doing a linear amount of database requests for
fetching all MessageTargets, we now cache them. The memory impact
should be negligible.
a980293a3dd2b0587743d2b0587743782082a396782082a396ba3f69d393WIP: Send METADATA messages on METADATA SUB after conn-regto Support METADATA for usersNow ready for review, works with emersion/goguma#18
Gentle ping ❤️
ba3f69d393407019d247Rebased.
407019d247cf87f7ecf3This patch applies cleanly without #55. Rebased to drop that extraneous commit.
@ -61,3 +61,3 @@GetMessageLastID(ctx context.Context, networkID int64, name string) (int64, error)GetMessageTarget(ctx context.Context, networkID int64, target string) (*MessageTarget, error)ListMessageTargets(ctx context.Context, networkID int64) ([]MessageTarget, error)I'm worried about the performance implications of this function. We only ever append to the
MessageTargetstable without ever deleting from it.This function would only be called when loading a network, and we'd cache its values afterwards.
Yeah, but this is an append-only list, so we'd load an increasingly long list on startup for each user.
I've pushed a fixup patch to stop keeping all message targets around in memory. Does that look fine to you?
@ -3521,0 +3528,4 @@dc.forEachNetwork(func(net *network) {// Technically we are only required to send the METADATA of channels we are joined to, and users// we are sharing a channel with or are MONITOR-ing, but it is easier to send all METADATA we are// aware of, and the spec guarantees that servers MAY send these messages at any time.To avoid the performance issues mentioned above, I think it'd be safer to just send METADATA for channels and monitored nicks here. Same about
welcome.We cannot do this because we need to send METADATA for "other clients in the channels they are joined to" https://ircv3.net/specs/extensions/metadata#notifications
Without a list, we'd need to query every single nick in common channels.
Ah, indeed!
I thought about iterating over user lists for all joined channels and monitored users, but it sounds like this would get quite complicated and inefficient.
I'm still not a fan of keeping state about all message targets. Do we really need to keep
messageTargetsaround?cf87f7ecf30fbb0d2fa3@ -1693,6 +1693,9 @@ func (uc *upstreamConn) handleMessage(ctx context.Context, msg *irc.Message) errCommand: msg.Command,Params: []string{dc.nick, target},})if online {Is there a reason why we need the network to be online to send metadata?
Actually, sounds like we should send metadata in the downstream handler for
MONITOR?It sounds like we already send meatadata in the downstream
MONITORhandler.@ -2692,0 +2712,4 @@}}}dc.sendTargetMetadata(ctx, mt)}No need to send METADATA on MONITOR + if we already sent that info on welcome?
@ -1696,0 +1696,4 @@// TODO//if online {// dc.sendTargetMetadata(ctx, target)//}OK, can remove this
168ab01b336f9d7cb2a5I've addressed these comments. Please test!
@emersion Tested, LGTM. Let's merge! 😁
Thank you!