search

squidowl / halloy

IRC application written in Rust
GNU General Public License v3.0
1.68k stars 66 forks source link

rfc: refactoring `Server` #640

Open 4e554c4c opened 2 weeks ago

4e554c4c commented 2 weeks ago

The Problem

When working on #77 I have run into numerous issues with how to represent a "server" in halloy. The Server struct, which is a wrapper around String, is currently ubiquitous in the halloy codebase, however it has many issues. Since Server is a String, &Server is &String, which is an anti-pattern. Variables of type &Server must be .clone()d often to satisfy lifetime issues and in order to satisfy lifetime constraints, which leads to the same "server name" being duplicated all over the codebase. For example, it must be cloned every time a message is received by the IRC server to communicate the server updated between the "stream" and "client".

In order to implement #77, the way that servers are referenced must be changed as well. In a bouncer-networks setup, several "primary" servers each set up any number of secondary servers. Thus there is a hierarchical relationship between servers, and a single config is shared between many. Furthermore, servers can no longer be identified by their name. Names may not be unique (two secondaries on under two different primaries may match), and are not the canonical way to refer to a server (the canonical identifier for a "secondary" server is its bouncer NETID).

Therefore, however we represent Server, it cannot also be used as the human-visible name for a server, which causes issues elsewhere in the codebase

A Proposed Solution

Ideally, in order to send a Server identifier with every single irc message, the Server type should be Copy (not just Clone!) and cheap to replicate. Furthermore, we should be able to get additional data that a Server represents by asking a centralized source of information.

My proposal is as follows: Server should be a Uuid (either v4 or v5). This data can be cheaply copied and sent between tasks without a performance overhead. Then, when it is necessary to look up server information (such as user-visible name) this can be gathered by looking at the a HashMap<Uuid, ServerInfo> which contains server config name, bouncer ID, bouncer name, etc.

CCs

@tarkah , @casperstorm let me know what you think. Last time I brought this up you mentioned that I should open an RFC before doing any huge refactors with Server and friends :smile_cat:

tarkah commented 2 weeks ago

Sounds reasonable! I don't even think we need a UUID. Since the server name from the toml config has to be unique, we can simply hash it and use that as the u64 id.

4e554c4c commented 2 weeks ago

Sounds reasonable! I don't even think we need a UUID. Since the server name from the toml config has to be unique, we can simply hash it and use that as the u64 id.

Yes, but the issue is that since bouncer-networks (#77) would mean multiple servers / connections for a single TOML "server name", we need a better way to identify servers

for example, two servers from two different bouncers could be called "libera"

tarkah commented 2 weeks ago

Makes sense 👍

4e554c4c commented 2 weeks ago

Ok i'll try to work on this separately from the bouncer-networks stuff (since that's already getting kind of complicated) and we'll see how it looks :tada:

4e554c4c commented 2 weeks ago

I think I'm going to try to make it a v5 UUID with a consistent hash function, so that we can use these in the history in lieu of server names