#Development Chat

1 messages · Page 2 of 1

silent marsh
#

seems like cache issue

jaunty jewel
#

yeah seems like it

silent marsh
#

probably cached some native target compiled lib on other cpu

#

and that is not compatible with this runner

jaunty jewel
#

hence the illegal instruction

silent marsh
#

ok cleared cache

#

let's see

#

uhm

#

why do we have broken intradoc links as warn and not deny

#

merge queue forces me to force push if i want proper commit messages, not cool

jaunty jewel
silent marsh
#

and the labeler is apparently confused because docs and fix are both in the commit msg

jaunty jewel
karmic aurora
#

Sure, but let's switch back when it supports custom commit messages

silent marsh
#

Thanks, I'll disable it for now then

jaunty jewel
#

👍

ocean nest
#

should we use a check to make sure theres no dead links?

silent marsh
#

we explicitly opted to setting clippy warnings to warn instead of deny

#

what surprises me is that this should've been annotated

ocean nest
#

should we use a check to make sure theres no dead links?

#

theres some actions that do that the only caveat iirc is that it cant check anchors

silent marsh
#

we could also just deny the clippy lint, problem solved

ocean nest
#

what lint does this?

#

theres one for intradoc links but i havent seen any for real links

silent marsh
#

yeah i meant the lint for intradoc links

ocean nest
#

also a lot of the links are broken on twilight.rs idk why

jaunty jewel
#

@ocean nest I'll compile a list of broken links

#

these are the only two I could find rn

#

Also, random idea: do we want a RELEASES.md similar to that of Rust's to track releases?

ocean nest
#

shouldnt it link to twilight-model

jaunty jewel
#

here

#

It does not even link to anywhere for me

#

It links to nowhere

jaunty jewel
#

any thoughts

karmic aurora
jaunty jewel
karmic aurora
jaunty jewel
#

idk I thought that didn't exist yet

#

but basically the same thiing

ocean nest
crimson sparrow
#

Btw what do you think about rewriting twilight.rs to make the documentation more accessible for new users? And why not put the release notes into a "blog" section?

jaunty jewel
#

The MDBOOK version seems... a bit boring I'd say

crimson sparrow
jaunty jewel
#

or could it be an all-in-one thing

#

just wondering

crimson sparrow
#

It can be the entire website

jaunty jewel
#

Yeah we can use that to build the new website

#

to be fair I feel like we should theme it so it is more... rusted

#

just my personal opinion though

crimson sparrow
jaunty jewel
#

or we can define our own colour scheme for the website

jaunty jewel
#

I think this is a great thing to use

crimson sparrow
#

I made a thread, it will be easier to follow

jaunty jewel
#

👍

jaunty jewel
#

cc @karmic aurora do you think we can take a similar approach with how we handle interactions which those endpoints also require an application ID?

#

i.e. a separate client dedicated for appliction role connections

#

and can be accessed via role_connections() in the normal client

#

also that PR 2004 seems to be stale for quite a while

ocean nest
#

why dont we just combine that with InteractionClient

#

theyre the same just different methods

jaunty jewel
#

ApplicationClient?

#

Because it is not only for interactions now

ocean nest
karmic aurora
#

Before the introduction of InteractionClient there methods returned an error in that case, but this is not necessary as it's strictly an programmer error

#

I can formalize this further / write up an RFC but haven't gotten around to it

ocean nest
#

i like the other way better cos its compile time checked

jaunty jewel
#

And I believe that’d be a 0.16 because it is strictly a breaking change

#

If we do decide to remove InteractionClient altogether

jaunty jewel
#

like a compile time error if the application id is not specified

#

but that's far reaching

#

and probably not possible

jaunty jewel
ocean nest
#

they’d basically be different types

jaunty jewel
#

I just finished my maths exam for today and I have 5 day offs ahead

#

Should have more time to write and draft the RFC

karmic aurora
silent marsh
#

isn't actions-rs dead

#

that might explain it

reef forge
karmic aurora
#

@silent marsh next thing on my TODO-list is the generic cache, just wanted to warm up a bit with some gateway PRs :)

silent marsh
#

I'll look into the PRs soon

#

btw why did you close the PR that wanted to put queue back to gateway

#

just interested, I don't think we should do it

jaunty jewel
karmic aurora
#

even if Twilight does 1 shard at just 300KB that's over 5GB with 20,000 shards

ocean nest
#

how does it take 300kb

wild marlin
#

Various buffers probably take most of that

#

For example we have a compressed and a decompressed buffer, and a zlib dictionary

ocean nest
#

ah how does gateway queue help this

wild marlin
#

not really

ocean nest
#

sad

#

is this buffer necessary

#

or just an optimization

wild marlin
#

for zlib yes

#

without zlib you can do it with one buffer

ocean nest
#

and it doesnt buffer that if u disable compression?

#

welp not much we can do then ig

#

5gb isnt that bad for a very large bot

wild marlin
#

It's going to be overshadowed by other parts especially cache

karmic aurora
#

300 is just a sample number

#

Don't know the true one

karmic aurora
karmic aurora
ocean nest
#

oh like horizontal still the same thing

ocean nest
wild marlin
#

The thing that makes it less necessary than when it was introduced is that buckets are much more in use now and you don't need this kind of synchronization between two different buckets

ocean nest
#

hmmm

#

i see

karmic aurora
wild marlin
#

only within each bucket

#

You can start bucket 1 and 2 at the same time for example

karmic aurora
#

no?

#

maybe we're talking past each other

wild marlin
#

So if you have 2 buckets (I think the smallest amount is 16, but for examples sake)
Then shard 1 will end up in bucket 1 % 2 and shard 2 will be in 2 % 2 right.
Then shard 1 and shard 2 may start at the same time, shard 3 will have to wait for 5 seconds after shard 1 and shard 4 will have to wait for shard 2. There is no need to syncronize between bucket 1 and 2.

silent marsh
#

i might be completely wrong but iirc there is no requirement for shard ids to buckets on discord's side

#

you can just wait for any bucket to be available

karmic aurora
#

Oh I see, we're conflating buckets and max_concurrency. You call buckets what I call max_concurrency.

#

I'd say there's room to send a bucket every 5 seconds with up to max_concurrency identifies

#

but yes, we agree

silent marsh
#

max_concurrency is a leaky bucket with max_concurrency tokens and max_concurrency refills per 5s

#

if you want to imagine it that way

wild marlin
silent marsh
#

that is undocumented?

#

at least i searched in the docs a few mins ago and didn't find it

#

i'm pretty sure it doesn't matter in which order you identify shards

#

you could just identify 0-15, then 16-31 and so on

#

ehh, i mean

#

0 + 16 + 32 + 48 and so on at the same time

#

sorry

wild marlin
silent marsh
#

ah thanks

karmic aurora
silent marsh
#

a bit funny that not even mason knew how the system works

wild marlin
#

It used to be completely undocumented back then

#

I was doing it a bit blindly when I implemented it for twilight as well

#

Especially since I cannot test it myself

silent marsh
#

we got max_concurrency raised to 16 around that time

#

that's probably why i wasn't aware of it

wild marlin
#

3 years ago blobsved

karmic aurora
#

// DISCLAIMER: THIS IS A VERY BAD QUEUE!

wild marlin
karmic aurora
silent marsh
#

i'll take a look today or tomorrow

silent marsh
#

the websocket lib PR turned out smaller than i thought

ocean nest
#

its actually good reduction of code

#

well its draft so early to tell

silent marsh
#

it works too

image.png
#

i didn't expect it to break or anything

#

i have to run a long term test on my main bot though to make sure i didn't break anything with reconnect handling

#

@karmic aurora by the way, feel free to message me directly if you need a token for a bot with max concurrency 16 to test the gateway queue, i wouldn't mind you using it for testing

karmic aurora
#

I've also looked at the popular C, C++, C#, and Go libraries and none of them offer have, from a quick glance over, obvious bugs that never the less don't present themselves in user reported issues

#

meaning, even if the queue is slightly wrong, I don't know if it'll be detectable

silent marsh
#

Discord is lenient with the timing so yeah should be fine

silent marsh
#

Does anyone here have a clue what to do with https://github.com/twilight-rs/twilight/actions/runs/5495920717/jobs/10015613965?pr=2239? The issue is that --no-default-features alone won't compile (intentionally), since --feature no-tls should be specified in that case (because cargo doesn't allow [target.'cfg(not(any(feature = "ring", feature = "openssl")))'.dependencies] to pull it in on the library end). I can only find relevant docs for cargo-hack and grouping features, not for just skipping --no-default-features or grouping it with --feature no-tls

wild marlin
#

It is because cargo features should not be negative

#

That usually breaks a bunch of stuff

#

This is again the annoying thing where cargo don't have support for something like enums

silent marsh
wild marlin
#

The obvious, but not great solution is of course to always include sha1_smol

silent marsh
#

yeah that's the only thing I can think of but it's not great

#

pulls in an unused dependency if TLS is enabled

wild marlin
#

But the opposite is not also great, eg you have no_tls enables but it still uses tls

#

It is all about tradeoffs, hopefully we will someday get a better way of doing it

wild marlin
silent marsh
#

i.e. if you enable two TLS backends it will prefer one

#

same here, if you enable no_tls and a TLS backend, it will prefer TLS

wild marlin
#

But is that the obvious thing?

#

That is the main issue I have, but I think it is up to you to decide on the best way

silent marsh
#

no_tls is a power user flag anyways

#

not going to be used by 99% of users

#

only relevant if you are proxying the gateway connection locally

#

that's also why I think we shouldn't enable sha1_smol by default, it's not going to be needed for the vast majority of dependents

#

i believe we should document the current precedence anyways if we haven't already

wild marlin
#

I think we have

silent marsh
#

we say mutually exclusive in the readme

#

which isn't technically true

#

but if we go by that, just adding no-tls to the list is enough

wild marlin
#

I think it may have been true when we wrote it

#

I think we used to have a compiler_error for it

silent marsh
#

could be that it was removed when we added the powerset ci

#

mutually exclusive features are a bit painful to maintain with powersets

wild marlin
#

They are a pain in general with cargo

silent marsh
#

problem is that it will probably still run --no-default-features --features zlib-stock e.g.

wild marlin
#

ahh yeah

ocean nest
#

is there a reason these are private ```rs

/// Forbidden substrings in usernames.
const USERNAME_INVALID_SUBSTRINGS: [&str; 5] = ["@", "#", ":", "```", "discord"];

/// Forbidden usernames.
const USERNAME_INVALID_STRINGS: [&str; 2] = ["everyone", "here"];

/// Forbidden webhook usernames.
const WEBHOOK_INVALID_STRINGS: [&str; 1] = ["clyde"];

the docs also says "certain strings" instead of linking to these constants
silent marsh
#

there's no reason to make them public really

#

might change anytime and is an implementation detail of validation

ocean nest
#

well it'd be cool to see it for yourself

#

which is the rationale behind making other consts public (i think)

#

for my use-case i wanted to access the invalid strings, get the one with the highest char count and ensure that the string im replacing the substring has lower than this char count so that i dont exceed the max length

silent marsh
#

I'm looking at implementing lavalink v4 for the client and I'm surprised by our current API

#

We don't have an actual lavalink client struct and instead the user has to pass the socketaddr for the server on every method invocation

#

I hope it's fine if I move that to a struct

jaunty jewel
ocean nest
#

why doesnt twilight validate that the attachment size is under 25 mb?

silent marsh
#

I would like to get that PR moving forwards, it's been pending for a few months now and I've received feedback from some other large bot developers that they're very interested in this

#

I think the way to go for now is probably to make the traits sealed and conditionally unseal them behind an unstable flag

#

Only question remaining then is if we wanna break the API or if we wanna move the generic InMemoryCache to CustomInMemoryCache or so and rename the DefaultInMemoryCache type alias to InMemoryCache. That would also decide whether this goes to main or next. I'd favor keeping the names as they are in the PR because that's what I'd like for the future

charred fable
#

I was going to work on it since it seems abandoned

silent marsh
charred fable
#

I see. I'd like to help as well, though I don't know much about lavalink in general.

charred fable
#

Let me know if you need some sort of lib tester though, I'd be more than happy to be one

silent marsh
#

I'll let you know once I have it in a usable state

karmic aurora
#

I'm switching to allow @\team to merge PRs that pass status checks (incl 2 approvals)

#

sorry for the ping 😅

jaunty jewel
#

👌

jaunty jewel
#

because it notifies us of the update in permissions

wild marlin
#

Or could they that already

jaunty jewel
#

I can approve prs

#

so I think that should be everyone on twilight-rs/team

#

SIGILL again

#

well

primal torrent
#

yeah don't cache rust target folders on github. their cpu range for the runners is too diverse and you have no control over it

silent marsh
#

@karmic aurora generic cache PR should be ready now i hope

#

renamed the module from interfaces to traits, renamed the traits and added the debug requirement

silent marsh
#

actually, I think I made a bunch of typos

#

it's Cacheable, not Cachable, right?

#

fixed :)

ocean nest
silent marsh
#

And instead target haswell

#

Which is the minimum for AVX2

#

And forward compatible with everything newer

lean geyser
#

weird question but why even bother building the docs with AVX2

silent marsh
#

good question actually

#

I think we use --all-features

#

which enables simd-json

#

which in turn won't compile otherwise

lean geyser
#

hm makes sense

#

probably not worth the hassle not turning it on vs just copy/pasting some target feature stuff around

karmic aurora
#

I think it's around time to cut a new stable release. Lot's of nice model additions have landed

eternal owl
#

can't elaborate but I would not use ImageHash for avatar decorations

#

I'd just make it a string

silent marsh
#

it's documented as one iirc

silent marsh
#

specifically talking about the gateway-queue rewrite and generic cache

#

that's probably stuff which should be in next for a while

jaunty jewel
#

I think we can cut a 0.15.3 today

jaunty jewel
#

yeah it is documented as one

karmic aurora
silent marsh
eternal owl
#

I would look closely at the documentation for what an image hash is, and think about what has been left open-ended

wild marlin
#

Unless avatar decorations have changed they are breaking the format

#

Iirc they start with v2_...

#

And that have been reported up the line, but it was not documented publicly so it did not matter for us then.

#

To my understanding image hash may in the future be documented as we use it, but they are not ready to lock into it at present time, so extra care should be taken with it.

charred fable
#

Is twilight-lavalink missing a TrackException variant in IncomingEvent?

silent marsh
#

@karmic aurora for CacheableVoiceState, do you want PartialEq<VoiceState> or PartialEq<(Id<ChannelMarker>, Id<GuildMarker>, VoiceState)>? The From impl requires the IDs

#

(this is one of the reasons why I think PartialEq makes little sense to require for all types)

#

same for CacheableMember, currently requires PartialEq vs Member, PartialMember, InteractionMember and now Self, but do i also require it for the tuple constructor variant?

#

i guess not, makes little sense to me

karmic aurora
#

yeah I was thinking of just the base type, without a tuple

#

we try to equate the items based on what's there

silent marsh
#

bit nasty to keep approving your CI runs :)

karmic aurora
#

:D

ocean nest
#

lemme pr this

silent marsh
#

that documentation sounds weird too

#

The vec count can be between 2 and 100

#

That could definitely be worded better

wild marlin
#

@silent marsh If I am not mistaken your lib will do UB if someone sends a close frame with only a single byte of payload besides the opcode (though it is probably disallowed by the standard)

silent marsh
wild marlin
#

Ahh makes sense, I was just looking around the unsafe places and see if there was anything that jumped out there

silent marsh
#

I am pretty sure all my unsafe usage is sound, but I wanted to add comments with explanations anyways (done now 🎉)

jaunty jewel
#

The .timestamp_millis method on Utc has been deprecated

image.png
#

The new recommended function is .timestamp_millis_opt which returns a LocalResult

lean geyser
#

that's so cursed

ocean nest
#

i hope they remove the panicking methods in the next release

#

i dont like either of the time crates in rust but it is what it is

#

and i dont hate myself enough to make another

silent marsh
#

you don't wanna deal with making a time crate

#

the amount of edge cases is unbearable

ocean nest
#

yeah its painful

#

would rather write c

#

or even dare i say js

jaunty jewel
ocean nest
jaunty jewel
ocean nest
#

do we need to update twilight for this

jaunty jewel
#

I don't think so

ocean nest
#

lets wait till someone has a complaint ferrisballSweat

ocean nest
#

in the docs this is said about referenced_message

This field is only returned for messages with a type of 19 (REPLY) or 21 (THREAD_STARTER_MESSAGE). If the message is a reply but the referenced_message field is not present, the backend did not attempt to fetch the message that was being replied to, so its state is unknown. If the field exists but is null, the referenced message was deleted.
but https://api.twilight.rs/twilight_model/channel/message/struct.Message.html#structfield.referenced_message is just one Option so is there a way to determine between the field existing and the field being null?

Message in twilight_model::channel::message - Rust

Text message sent in a Channel.

karmic aurora
#

Another example

{
  "avatar_decoration": "v2_a_37dc2b53b273a457ff19ac2e3fda7e4c",
  "avatar": "a_76ffadf702eec2a8d2396a0651225179"
}
#

I guess we need to add a version: u8 field to the image hash

jaunty jewel
wild marlin
#

I already knew about that issue

#

I think it is better just to have that as a string until Discord figures out what they want with it.

karmic aurora
wild marlin
#

Guess we have to make a fix pr then

karmic aurora
#

The fact that they're keeping the 24byte base64 encoded hash seems to me that the format is pretty stable

wild marlin
#

Yeah I have been petitioning for them to make it stable, but that one field is strange

#

It was fine in the start because the field was not documented, but it is now I guess

jaunty jewel
karmic aurora
#

Yes!

jaunty jewel
jaunty jewel
ocean nest
silent marsh
#

#2179 could use some reviews to get it merged, as I mentioned a while ago, it's painful to resolve conflicts because almost every cache change will conflict with this PR

karmic aurora
jaunty jewel
#

so I think it is normal that you don't have push access

#

That has happened before when cassandra took my other PR forward

karmic aurora
#

hmmm, how should we resolve this then

#

How did you work it out last time?

jaunty jewel
#

Cassandra opened a new PR

ocean nest
wild marlin
silent marsh
#

That's an example dependency only, yes

#

And we aren't affected by this either since we don't use that part of the API

ocean nest
#

wouldnt hurt to upgrade it i guess

karmic aurora
silent marsh
#

The changelog generator is doing some very weird stuff

#

Ahh had to pull tags

silent marsh
#

@gaunt pollen could you take a quick look at the changelog PR?

#

🎉

#

Next release process will be smoother, I had an oopsie with the http-ratelimiting publishing permissions and only noticed an issue in model after I already published and started writing changelog. Next time I'll write changelog before publishing.

silent marsh
#

Just rebased next onto main and fixed the CI errors

primal torrent
#

i'll try looking at the cache one tomorrow. prob gona skip the websockets one cause i don't know anything about websocket internals and not done much with them directly

silent marsh
#

the websockets one is pretty much a no-op one because the apis are very similar

primal torrent
#

it's more of a "i have no idea what it was doing, nor what the new code is exactly doing"

karmic aurora
#

Thank you for the release!

silent marsh
#

🎉 i managed to do it

# 
2023-09-12T23:37:47.699617Z DEBUG shard{id=[147, 240]}: twilight_gateway::shard: gateway connection closed
thread 'tokio-runtime-worker' panicked at /root/.cargo/git/checkouts/twilight-c0eace12c9cff926/5cbb3ab/twilight-gateway/src/shard.rs:729:30:
internal error: entered unreachable code: stream ended because websocket is closed (received close frame sets status to disconnected or fatally closed) or because it errored (which also sets status to disconnected)
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
ocean nest
primal torrent
#

can't see that, but if it's only used in examples shouldn't it be trivial to just update it?

#

so we don't recommend an old version that will set off dependabot in their repository then if they use it?

silent marsh
#

I dismissed the alert, but a PR for updating the example dependency is welcome

ocean nest
#

examples need an overall haul (iirc) but i dont have the bandwidth rn sorry

primal torrent
#

can we maybe make an issue for it then? then people like me who don't have access to the security panel can take a look at it as well

silent marsh
#

the vulnerable code isn't used in the examples anyways though

#

I'll submit a PR

primal torrent
#

¯_(ツ)_/¯

silent marsh
#

teams with the security manager role
hmmm do we want to grant this to twilight-rs/team?

ocean nest
silent marsh
#

that's what I did

primal torrent
#

even if it doesn't affect twilight, the examples are so simple we might as well take care of whateve it is so people running scanners on dependencies don't get pointless false positives

primal torrent
jaunty jewel
#

Interesting replacement for dashmap if we use it

ocean nest
#

interesting

#

another (non rust) lib im using also uses async for caching

#

but sometimes async can be less performant

#

i think wed need some benchmarks

#

itd probably be beneficial for finding a message out of millions but be overhead for finding a guild from thousands

silent marsh
#

the main issue with anything that isn't dashmap that has sprung up so far is that the algorithms only held for primitives that are Copy

#

anything else was not lock-free

#

and the main issue with dashmap was always deadlocking

#

performance wise it's pretty good

#

haven't looked at this crate yet, but my hopes wouldn't be very high

ocean nest
#

there was one that used GC

#

idk how good it is but maybe itd work

wild marlin
#

@karmic aurora @silent marsh re: Investigate static HTTP form boundary
I am pretty sure that would open us up to possible odd attachment thing happening, at best just half attachments being added if they craft it in a particular way

#

That is the reason it should be randomized

#

We would like have to add a test that the boundary does not exist anywhere in the file and then the change is a bit moot.

silent marsh
#

in doubt, make the boundary longer so the chances decrease rapidly

wild marlin
#

Which is the main reason I am against it.

#

It is not something that is either timing sensitive or thruput sensitive so I don't think it is too much to have a source of randomness.

#

Sure it may not have to be a very secure source, but it should not be static

silent marsh
#

we could generate a random boundary at compile time

#

via a build script

#

iirc this is the only use of rng in twilight-http

wild marlin
#

I don't think it is necessary to code golf that dependency away

silent marsh
#

it's definitely possible and I don't see why not really

primal torrent
#

does it really matter either way? does it really have a postive or negative impact for the effort?

silent marsh
#

the build script is going to be 3 lines of code that save us a dependency at runtime

wild marlin
#

It gives a unmeasurable speedup and may make the compiled binary smaller

silent marsh
#

the binary size difference will only come into play when gateway isn't depended on

#

since the websocket always needs a rng for masking

#

to be honest, static frame masks are somewhat possible, the only reason why I think masks should be considered is xor'ing the bytes around for better compression on lower transport level but that's mostly pointless in my view

#

since the mask is always sent there's no security downside I can see to using the same mask over and over or even a no-op null byte one

primal torrent
#

replay attacks and easy brute forcing in targetted attacks?

silent marsh
#

if you have the full websocket message, you can unmask and remask it yourself

#

so yeah pretty pointless for most cases apparently

#

it's only to prevent intermediate proxies from misinterpreting the data

primal torrent
#

ah alrigh

#

not sure how common it is for proxies to be present

karmic aurora
silent marsh
wild marlin
#

yeah

karmic aurora
#

Attachments are base64 encoded right? Can we then not use non-base64 characters in the boundary? (since all ASCII chars are valid boundary characters)

wild marlin
silent marsh
#

Is there anyone who could review the PR #2239 or is vilgotf the only one who feels like they have enough knowledge of the gateway code to be able to approve it?

#

I'm just wondering if people haven't had time or if it's an issue with not feeling knowledgeable enough to review it

#

In the latter case I would just merge it

#

If it's the first case let me know so I can wait for your reviews

wild marlin
#

I can have a look after work today

knotty ferryBOT
#

refactor(gateway, http, lavalink): Switch to tokio-websockets, authored by @Gelbpunkt <t:1688747296:R>

This switches our websocket library from tokio-tungstenite to tokio-websockets.

Why?

  • tokio-tungstenite is notoriously slow with dependency updates…
wild marlin
#

@silent marsh I think it would be nice to get it out in a if not pre-release then a test release so we have something we can point people towards when it comes to testing

karmic aurora
#

Pre-releases are always nice, but the websocket library has already passed thorough testing by me and Adrian so I don't think that will cause any problems

silent marsh
#

For the next minor release we can do pre-releases when the majority of breaking changes is merged

#

IIRC we're missing the websocket library change, generic cache, cancelation safety and ideally lavalink uprev

karmic aurora
#

I'd like to land a http-ratelimiting rewrite too, but I've been procrastinating it for a while now so we'll see about that

silent marsh
#

My work on the lavalink uprev is in very early stages anyways, will take a bit until that's done

#

Gonna merge the PR now, I've had it running on 240k guilds for a week now without issues

#

The library has been tested so much I doubt we will see any issues that arise from it

#

up to 40% more performance under high load is nice :)

opal juniper
#

GitHub gives us the option to merge our own PRs after it's been approved. Are we allowed to merge our own PRs?

silent marsh
#

Feel free to merge them if they have two approvals

wild marlin
#

@silent marsh Actually I had one question regarding your WebSocket library, why do you write it as Websocket when in the standard and such it is WebSocket

silent marsh
#

because I always write "websocket" and Websocket is the equivalent type name

#

never really put much thought into it though

#

I find it less annoying to type and read as well since it feels more natural if I'm usually writing "websocket", but yeah keeping the rfc naming wouldn't be bad I guess xD

ocean nest
opal juniper
#

Yeah I was asking on behalf my own PRs

jaunty jewel
#

Do we want to use Id<GenericMarker> for this?

#

Indeed, we can use sum types for this as well

opal juniper
#

does twilight support the newer select menu types? If it doesn't that should probably implemented first or have default values support within the PR

ocean nest
#

there was a pr for it but i dont remember if its merged

primal torrent
#

is there any threads or progress on unifying the api twilight exposes? the inconsitencies are very annoying to have to work around

jaunty jewel
opal juniper
wild marlin
#

I guess the dictionary got updated

primal torrent
#

ok so this is a weir quirk, cargo expand can actually hide errors >.< just pend 2 hours hunting this error. looks fine right?

image.png image.png
#

except that's it has actually replaced the faulty token with a good one

image.png
wild marlin
#

Is it only for the close message you need the mut part?

silent marsh
#

Message payloads are internally stored as an enum with Bytes and BytesMut variants

#

That is because for masking the payload, having a BytesMut allows doing it in-place

#

Whereas using Bytes means we have to allocate and copy it to mutate it

#

This means that messages can be cloned cheaply at any time (since Bytes is a cheap clone and BytesMut can be converted to Bytes for free)

#

But the conversion from Bytes to BytesMut isn't possible

#

Taking a BytesMut allows us to make sending zero-copy

#

If you have a Bytes and want to send it, having an API that takes Bytes would be the same overhead if you converted it to BytesMut yourself and use the current APIs

wild marlin
#

Could masking not be done when reading it for sending or would make the performance a lot worse

silent marsh
#

when reading it for sending
what do you mean by that

#

writing to the websocket requires a buffer to write from, so you need the masked payload as a buffer

#

BytesMut is the most efficient way to do this because it allows xor'ing the buffer in place

wild marlin
#

Have a custom read impl that does it

#

(is what I meant)

silent marsh
#

The masking key has to be random for each sent frame, so you're not allowed to mask once and send the same bytes to multiple peers

wild marlin
#

I mean if you have something like:

struct Frame {
  data: Bytes,
}

impl Read for Bytes {
  fn read(&mut self, buf: &mut [u8]) -> Result<usize> {
    // do masking here
  }
}

or do you not think that is feasable

silent marsh
#

Bytes is immutable

#

You can't change it, even with &mut

#

I'm not sure what that code is supposed to do

wild marlin
#

Okay give me a bit and I will have a proper look

#

Hmm would it not only need to be mut when you are the client?

#

But I guess it would mean that you would have to split it up into client server pairs

silent marsh
#

But differentiating makes the APIs much more complex

wild marlin
#

Would it be possible to do something like Cow where it is only turned into a BytesMut if it is needed?

silent marsh
#

That's what we already do

wild marlin
#

Am I reading it wrong then, are you not always making a BytesMut if you use one of the Message constructors

silent marsh
#

Yes, we currently don't have constructors for the shared variant

wild marlin
#

Could it be something like

trait IntoPayload: Into<Bytes> + Into<BytesMut> {}

actually I am not sure that would be the best way

silent marsh
#

I think we should implement From<Bytes> for Payload and make the methods take Into<Payload>

wild marlin
#

Yeah that would probably be cleaner

silent marsh
#

payload is still private and I don't want to expose it

#

so probably sealed trait

#

better way to go

silent marsh
#

feel free to PR that

wild marlin
#

At least when you are acting as a client

silent marsh
#

yeah

#

I mean there is a weird case with always masking or never masking that I will eventually just make it configurable

#

Some websocket libraries just don't mask at all or always mask

#

I will eventually have to make it configurable on both ends

karmic aurora
#

I'm late, but note that the Clone impl is ~just Bytes::clone

wild marlin
#

Does that not still allocate if the inner is not a shared bytes

silent marsh
#

The clone does not allocate

#

Sending a cloned message will

silent marsh
#

i should ensure backwards compatibility with Into<BytesMut>

#

will do that later

#

ah, not possible

# 
impl<T> From<T> for Payload
where
    T: Into<BytesMut>,
{
    fn from(value: T) -> Self {
        Self::from(value.into())
    }
}

conflicts with From<Bytes> :D

#

so this will have to go into the next minor release

wild marlin
#

Made a small comment, but it looks good

opal juniper
#

Does anyone have access to premium apps? As of now I have no means of testing #2282

woven elbow
#

I can look over it next week if nobody else has. I implemented it all myself in the alpha so I have a decent grasp (I think)

opal juniper
#

Ok sounds good, it looks like they merged a couple of additional changes in the docs so I’ll get those implemented and ping you

jaunty jewel
#

PR 2288 needs reviewing

silent marsh
#

I would like a docs PR first

jaunty jewel
silent marsh
jaunty jewel
silent marsh
#

Issue is that it's not a Discord employee and it's nowhere close to merged

#

So they technically could change this behavior again

#

Because it isn't documented for devs yet

jaunty jewel
#

But the thing is that being null would break twilight itself deserializing

silent marsh
#

I'm aware but I also don't like band-aiding it

#

If we change this we should have some degree of assurance by Discord that this is correct and will stay so (in the form of a docs commit)

jaunty jewel
#

To be fair, I don't feel like breaking end-user code just because Discord hasn't documented something on their end; it is also a Discord problem for not communicating with devs in advance that this would happen.

silent marsh
#

you're breaking end user code either way here

#

either by keeping it as is and events failing to deserialize or by making it an Option and creating type mismatches

#

my point is that I want to avoid breaking it twice because Discord revises the API for this feature again

silent marsh
#

thoughts on doing the MSRV run with minimal-versions?

#

see my new PR for reason as to why

karmic aurora
#

+1 from me, combining them seems objectively better as the check becomes more thorough

primal torrent
#

basically every lib handles it, because while the official response usually is "it shouldn't happen" or it was things like "stage guests" when that was an experiment. the reality is you'll see these guaranteed from time to time at scale

opal juniper
silent marsh
#

@wild marlin the gateway-queue github container package has been private all this time and went unnoticed

#

it's impossible to pull it without being logged in xD

#

http-proxy is public... but idk how

#

I'm gonna allow members to push public packages

#

That allowed me to make it public

wild marlin
silent marsh
#

what is our current plan for raising msrv to 1.75? async fn in traits would allow changing the public API of the gateway-queue and any other instances where we use it

#

i know it's not even out yet, but we don't really have rules for what qualifies as a possible MSRV and always went with the flow

#

i'm not sure how quickly the ecosytem will adopt it, but I believe at least a handful of crates will switch to 1.75, i'm just not sure how fast

lean geyser
#

might be worth waiting till 1.76 to bump to 1.75 in case theres any unnoticed bugs in the async trait fn implementation that slow adoption

silent marsh
#

lol

silent marsh
#

it's unbelievable how many minvers/MSRV violations i am finding with this

lean geyser
#

MSRV is a scam sold by big debian anyway

#

most people dont take it seriously

silent marsh
#

xD

#

why the hell is this even testing dev-deps?

#

hmm i think we should drop --all-targets

#

MSRV checks in dev-deps are pretty useless

#

wonder how this was not caught by our CI ever before lol

#

ah, yeah we never checked minvers on --all-targets

#

TBH if we don't check minvers we might as well not check MSRV at all on tests/benches

#

aaand even with that our minvers + msrv is broken XD

# 
  error[E0554]: `#![feature]` may not be used on the stable release channel
    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/itoa-0.4.1/src/lib.rs:13:31
     |
  13 | #![cfg_attr(feature = "i128", feature(i128_type, i128))]
     |                               ^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = help: the feature `i128_type` has been stable since 1.26.0 and no longer requires an attribute to enable
  
  error[E0554]: `#![feature]` may not be used on the stable release channel
    --> /home/runner/.cargo/registry/src/github.com-1ecc6299db9ec823/itoa-0.4.1/src/lib.rs:13:50
     |
  13 | #![cfg_attr(feature = "i128", feature(i128_type, i128))]
     |                                                  ^^^^
karmic aurora
#

Bumping dependencies more than technically required is preferable to specifying transitive dependencies as that becomes unmaintainable real fast (and I've heard it might mess with cargo's build order graph)

#

But to your point, minvers is incredibly frustrating to deal with since fixing one dependency may unearth another issue. Additionally it's not uncomon, as with itoa 0.4, to pull in ancient versions. IIRC I managed to pull in a dependency from 2015 that didn't even build due to a breaking change in rust when working on the original PR for minvers

silent marsh
#

FYI hyper 1.0 is due in 2 weeks

silent marsh
#

like e.g. I could bump hyper to the first version on itoa 1.0

wild marlin
#

Can anyone see why the build docs failed?

#

Else I will have a look when I am home

primal torrent
wild marlin
#

That pr should fix it

#

For some strange reason I do not get that error locally which is annoying

wild marlin
#

Thanks

wild marlin
#

@swift gull You can rebase your pr now

swift gull
#

Thanks for the ping!

#

Oh, your PR targets main. What's the normal flow for porting fixes to next?

wild marlin
#

Ahh, I will have to look at that, idk what they do atm I think @silent marsh or @karmic aurora have been doing it

karmic aurora
#

I locally merge main into next, run som tests, and yolo admin push it

wild marlin
#

okay will try that then vikingblobsved

#

I should fix that

#

@swift gull now you can

swift gull
#

Thanks again. That fixed the CI!

eternal owl
#

how yall doin

lean geyser
#

Work is hellish busy but outside of that fine. You?

wild marlin
#

Hmm did I cherry pick that commit incorrectly

#

Should probably just have merged

#

It seems to think that the main commit and the next commit are two different ones

silent marsh
#

cherry-picking tends to mess up merging yeah

wild marlin
#

Should I fix it with a force push and then merge it with a merge commit instead?

silent marsh
#

i am okay with force pushes to next

jaunty jewel
#

Have there been any progress on migrating to hyper v1

wild marlin
#

This move also involved moving http to 1

silent marsh
#

everything else is blocked on that

#

migration is rather straightforward for us so I will submit a PR once the hyper-rustls one gets merged

wild marlin
jaunty jewel
#

Thanks!

#

We might as well get a release out with the migration to hyper v1 perhaps

silent marsh
#

probably not

#

there's some major api changes still pending

#

I should get back to my open PRs once I find some time on the weekend

silent marsh
#

I would be open to an alpha pre-release to open up for wider testing of our current changes though

primal torrent
#

i think you worry too much about holding back changes

#

it's still 0.x

#

all it really does is make the eventual update even bigger and daunting rather then incremental

jaunty jewel
#

^^^^

silent marsh
#

That's why we decided to slow down on releases with breaking changes to allow doing more breaking changes at once, but less often overall

primal torrent
#

yes but then they happened every few weeks almost

#

now it seems to slowed all the way to once a year

#

that's swinging to the other extreme

#

0.15 was 9 months ago

silent marsh
#

Time alone isn't enough of an argument here

#

If you look at the amount of changes in next, it's not really enough breaking changes

#

Meanwhile we have like 2-3 pending things we want to get in for a new release

#

hyper v1, generic cache and ratelimiter rewrite are the major ones I'm looking at

primal torrent
#

but are they actually breaking? iirc we don't expose hyper

#

its all wrapped

silent marsh
#

I haven't had time to work on the cache PR since you reviewed it but I'll get to it soon

silent marsh
#

putting that into a patch release is not an option to me

primal torrent
#

ah, not sure if thats an issue but get the point

silent marsh
#

It won't break, sure, but it shouldn't really be done either

#

Users should upgrade all their dependencies to hyper v1 explicitly

primal torrent
#

rate limiter might be best to get its own release

#

like release right before that and put that as a pre-release for testing

#

cause its so high impact if it misbehaves

silent marsh
#

part of why I want a pre-release for the current things is the gateway changes

#

to my knowledge I'm the only one who tested it

#

it would be good to get confirmation from others that it didn't change behavior

#

edge cases happen and I'd like to avoid releasing something that's broken for some but works for me™

primal torrent
#

prob best to get a separate pre-release for that then

silent marsh
#

offtopic, I'm slightly annoyed that simd-json is dropping a new release every second week or so

#

keeping up with the version numbers in twilight is getting a bit tedious

primal torrent
#

does it actually do something significant each time?

silent marsh
#

internally it got safer

#

the api we use is untouched

primal torrent
#

ah, not bad to keep up then but see why that would be annoying

silent marsh
#

iirc our range is like 0.4-0.8 or so

#

I would suggest bumping the minimum for a new release of twilight

#

in case we need new APIs in the future

#

I wouldn't want to be restricted by supporting some ancient version

#

If you're fine with it I will make a list of things that I'd like to get included for a pre-release of 0.16 that we can then work down

#

ratelimiter rewrite is probably out of scope if we want to get it out soon

#

that's fine with me

primal torrent
#

yeah

#

bumping the min wouldn't be bad as well to ensure people are using a safer version like we used to test

silent marsh
#

did anyone here keep track of the api docs repo

#

aka are we missing anything significant

primal torrent
#

i didn't so not sure

silent marsh
#

I don't keep track of those anymore so idk

#

would perhaps be useful to set up some monitoring that we can then "tick" commits off when they're implemented or no action is needed

wild marlin
#

I have fixed next

#

In a path like /guilds/{guild.id}/members/{user.id}/roles/{role.id} is the ratelimit only dependant on the guild id?

silent marsh
#

ugh isn't that undocumented

primal torrent
#

Top-level resources are currently limited to channels (channel_id), guilds (guild_id), and webhooks (webhook_id or webhook_id + webhook_token)

#

except for emoji as listed lower due to abuse

karmic aurora
#

and other undocumented things, for example I believe message fetching in the same channel uses different ratelimits based on the message's recency [1]. But Discord has said that those things should be on the way out (said some years ago, without any update)

primal torrent
#

its also just the same bucket afaik, just the time till removed from the leaky bucket thats longer

silent marsh
#

@wild marlin can we get the clippy lints fixed asap?

#

i see your pr only covers a few

#

it kind of blocks ci for the other PRs

#

apparently simd-json also reduced their MSRV again which is good, means CI passes for that PR again

wild marlin
#

That should be all of them

silent marsh
#

Things I want to get in for 0.16 release candidates:

  • #2297 (clippy fixes, needs more lint fixes)
  • #2289 (simd-json bump, ready to go after clippy fixes)
  • API catch-up (#2284, #2283, #2282, #2235, maybe #2187, #2152, need review/more work)
  • #2253 (MessageSender renaming, but without a patch release in 0.15 to deprecate the old names)
  • #2250 (Document partial cancellation safety, needs rewording/extension)
  • #2309 (poll_next implementation)
  • #2249 (Make Queue generic, only if @karmic aurora wants to finish it soon, otherwise postpone to 0.17. PR looks mostly ready to review though?)
  • #2224 (Cleanup reshard example)
  • #2179 (Generic cache, ready for review)
  • #2159 (Global rate limit bucket, only if the author wants to continue it, otherwise postpone to 0.17 with ratelimiter rewrite)
  • #2162 (RequestReactionType to model, still needed?)
  • hyper v1
  • rustls 0.22
  • #2308 (rename native feature to native-tls)
silent marsh
wild marlin
#

Had a old rustc version when I did that pr on the train

silent marsh
#

Most of the PRs look ready or only need a final touch

#

I would appreciate if people who authored these could let me and others know what can be reviewed immediately and whether they are willing to finalize the PRs or if someone else should take over

silent marsh
#

i mean in the end it does not matter too much but CI fails on main too

wild marlin
#

Not other than that was the brach I made it on, I can change it to target main if you rather want that

silent marsh
#

unless it has conflicts it should probably reside in main

#

if it has conflicts i'm fine with just getting it to next for now

primal torrent
#

easier to merge into next from main then the other way around

silent marsh
#

yeah but we will merge next into main with the release candidates anyways

#

and i hope that will be in at least a week or max. 2

#

don't think we'll do patch releases before that

primal torrent
#

yes but that might not be right now, and best to get clippy happy on main before then

silent marsh
#

but i would really prefer a working CI on both branches

#

yes

wild marlin
#

rebased

#

I will just merge it back afterwards

silent marsh
#

and another +1 would be awesome so we can merge it right now

#

otherwise i will force merge

primal torrent
#

leme get on my pc and i'll look at it in a sec

wild marlin
#

looks at @primal torrent

primal torrent
#

approved

wild marlin
#

And merged into next as well

#

so both CI should run now

silent marsh
#

i will re-trigger ci on the simd-json bump pr

#

that one is ready as is

#

i will keep the pinned message up to date with what is done

opal juniper
silent marsh
#

looking at it right now

silent marsh
#

@woven elbow Hi, I vaguely remember you saying you had access to premium apps a rather long time ago and you have left some comments on #2282, would you be interested in taking over the PR from @opal juniper, addressing the pending reviews and testing it? See here for details

wild marlin
opal juniper
#

oh nice

silent marsh
#

https://github.com/twilight-rs/twilight/pull/2179#discussion_r1405208152 @primal torrent re: this. I agree there are places where it makes some sense (like (Id<UserMarker>, InteractionMember, ComputedInteractionMemberFields), but in some cases like (Id<UserMarker>, PartialMember) I think it doesn't. It's only really boilerplating and doesn't help implementors when the meaning of each tuple member is self-explanatory

#

In the end we can't really make the fields private since we have to allow destructuring them (which our reference implementation does as well), so it's only really a very thin wrapper that IMO i find it hard to find struct names for

#

I have moved (Id<UserMarker>, InteractionMember, ComputedInteractionMemberFields) to a single ComputedInteractionMember struct since it makes sense when ComputedInteractionMemberFields is already a cache-only struct, but I don't see the argument for (Id<UserMarker>, PartialMember) and (Id<ChannelMarker>, Id<GuildMarker>, VoiceState), which are the only other two cases where this tuple-From requirement is added

silent marsh
silent marsh
#

FYI I'll open a draft PR for rustls 0.22 later today now that it's been released and once hyper-rustls 0.25 is out we can merge it and I'll tackle hyper v1.

#

I'd really love to get a new minor release out this month so if we could get some reviews on PRs in the pinned message that would be awesome

karmic aurora
#

I'll try and fast track my half-done Shard::poll_next rewrite then 😅 (the gateway is semi broken on next, a bit due to putting of some fixes as "I'll fix that in the rewrite")

silent marsh
#

We're likely not going to be able to make it cancelation safe in the near future

karmic aurora
#

a poll_next impl implies cancel safety, that PR can be closed and will be superseded

silent marsh
#

.< true

#

I'll happily take a look when it's ready

silent marsh
#

i updated the payload PR, should be good to go now and then i would like to cut 0.5

#

if i were to rename the struct, it would be now

silent marsh
#

woops ignore that branch push please

#

someone pushed to the wrong remote

wild marlin
#

I have implemented the User Apps api in twilight and it seems to work fine.

#

Though it made me aware of something I am not a fan of, Discord have begun to use arrays of integers instead of bitmaps, I think there is also a example of that in the automod feature. So I think we may want to rewrite those to bitmaps? What are other peoples thoughts on that?

jaunty jewel
#

Kind of like a wrapper

#

As in how Id for me is a type-safe wrapper for Discord IDs

silent marsh
wild marlin
#

Also I think we should rename AnonomousId to ZeroableId or similar, since there as it looks currently going to be one more place where it can be 0

#

Where it means something different as well

silent marsh
#

sounds fine

silent marsh
#

i'd love to merge #2249, it needs one more review. anyone up for that?

knotty ferryBOT
#

feat(gateway)!: turn Queue into a generic type, authored by @vilgotf <t:1689701263:R>

By making Config, and its dependants, generic over the queue we avoid an unnecessary pointer indirection through Arc<dyn Queue> which has been a pet peeve of mine for some time. This furthers allows dropping the Debug requirement from Queue, another…

silent marsh
wild marlin
#

Nah I think it is, I assume it just bitrotted at some point

silent marsh
#

@wild marlin and @karmic aurora: i gave you two invites to crates.io for tokio-websockets so in case I ever go MIA (hope I don't) you have full access to the repo and the publishing

wild marlin
#

I think adding the Arc<Queue> was one of the first things I did years ago

silent marsh
#

i remember back when it was Arc<Box<dyn Queue>>

wild marlin
#

Was it that in the start?

silent marsh
#

i don't remember exactly

wild marlin
#

It was, it actually started out as Arc<Box<dyn Queue + Send + Sync>> but I think that was before you joined

#

Back before it was named twilight

karmic aurora
#

finally went through four months of old GitHub notifications...feels nice to be able to use the notification indicator once again

wild marlin
#

I really wish you could do negative matches in it

wild marlin
#

@karmic aurora did we ever merge that fix you did with dead shards in?

karmic aurora
#

No, but I think my in progress Stream impl will fix it

wild marlin
#

It was mostly if it is worth to add to main and release a new 0.15

silent marsh
#

@karmic aurora mind if I help you with anything? I have some time coming up and could e.g. rebase #2224 for you

karmic aurora
#

Sure. I can also publish the in-progress stream rewrite I have (later today, I'll notify you). I'll be bussy until atleast tuesday

silent marsh
#

Awesome, I'll take a look later then

primal torrent
silent marsh
jaunty jewel
silent marsh
#

will do that then :)

opal juniper
#

I’ll try to polish up my premium apps PR this weekend. Nice that they gave a way to test it now.

silent marsh
#

i keep pushing to the wrong remote, aaaa

#

since hyper-tls 0.6 it supports handling negotiated ALPN so this should actually work now

#

on a different note, skeptic now has a MSRV of 1.70

#

which causes MSRV builds to fail for me

wild marlin
#

Fun fact I think discord have disabled http2

#

(Maybe it has been enabled again not sure)

wild marlin
silent marsh
karmic aurora
#

--workspace --exclude book should do the trick

silent marsh
#

will give that a shot later

silent marsh
jaunty jewel
#

I got Hyper HTTPS working in 1.x without using the legacy Client

wild marlin
#

Yeah that is how we used to do websocket connections as well

jaunty jewel
silent marsh
wild marlin
#

What is missing in your pr to hyper-rustls?

silent marsh
#

basically i'm waiting for a review

#

the maintainer is a bit busy at the moment

#

but i'm in contact with them

wild marlin
jaunty jewel
silent marsh
#

by the way, we have a couple of PRs that are stuck because of a lack of reviewers

#

if they end up not getting reviews until next year i'm gonna just force merge them

#

@wild marlin it's time for a merge of main into next and then we can follow-up on that merge

#

the pr you just approved can be enhanced with some of the changes in next :D

wild marlin
#

So it should be merged and then a new pr targeting next right?

#

Also I think we might soon want to release next, possible a final release of main first though

silent marsh
silent marsh
#

i have a list of PRs i want to get merged before a release of next

#

I pushed a merge of main into next

wild marlin
#

It is more that I think we should freeze development on 0.15 (unless we have a good reason to go back and fix something)

#

But maybe that can wait until we at least have the hyper change

silent marsh
#

Yeah keeping main and next in sync is painful currently

#

Aaaand there goes the skeptic failure

#

I'm not really sure what the implications of --all-targets vs --workspace are

#

IIRC all-targets also means tests

wild marlin
#

I think workspace is the same as the deprecated --all and yeah the difference is the tests and examples which I am not sure is included in that.

silent marsh
#

I think minvers on examples is probably also a bad idea

#

So this sounds fine

#

oops that needs some wording fixes

silent marsh
#

vilgot pushed the WIP poll_next to a branch a while ago, not sure what the status is on that now

#

ugh seems like we need clippy fixes too

wild marlin
#

Ahh yeah 1.75 just got released

silent marsh
#

(or not force merge and just wait for a review)

jaunty jewel
jaunty jewel
#

Cargo-platform now requires 1.70

#

So all MSRV currently fail

#

@silent marsh shuold we bump msrv to fix this

silent marsh
#

no, there is an open pr to exclude book from msrv check to fix this

jaunty jewel
#

Alrighty

#

Left my review

#

Except Clippy currently fails on that

silent marsh
#

no point to make two entirely unrelated changes in one PR

jaunty jewel
#

alright

silent marsh
#

we have two options for the clippy fix: either ignore the lint or rename the field to something like stream or inner

#

side note: I'm in favor of dropping twilight-lavalink for 0.16 and reintroducing it again at a later point with support for the latest Lavalink versions

#

currently it is a pointless maintenance burden with an outdated codebase that urgently needs a complete rewrite and I doubt anyone even uses it considering it doesn't work with newer Lavalink versions

#

pretending like we are even remotely providing support for it probably isn't a great idea, the crate is only getting dependency updates from us

#

additionally my personal opinion is that lavalink shouldn't be a first party crate but I don't mind it. I do however think that no matter the future of the crate, we should admit the current maintenance state of it

jaunty jewel
#

(I have never used lavalink because I don't do voice stuff)

#

but it does seem like the lavalink twilight used is.. well

charred fable
#

I've been using the Lavalink crate for my music bot as is (with Lavalink 3.7), and honestly I don't think I'm missing out on much.
The only slight issue I had with it is that it doesn't handle TrackException. Apart from that, everything else introduced in the 4.0 Lavalink is rather optional for most music bots IMHO: Stuff like speed and pitch controls, equalizer support, etc

spare viper
#

Hello dear Twilight authors! May i ask you why you have chosen to use Id<MarkerT> instead of a newtype for every resource id? I was very impressed by this technique and decided to implement it in another api wrapper i regularly contribute to (disnake, Python), and in our case we found out that using newtypes saves quite a few text in usage (though this may be just Python specifics). For example with markers:

MarkerT = TypeVar("MarkerT", bound="BaseMarker")

class BaseMarker: pass
class ApplicationMarker(BaseMarker): pass
class AttachmentMarker(BaseMarker): pass
class ChannelMarker(BaseMarker): pass
# and so on for every resource type

class Id(int, Generic[MarkerT]): ...

# Usage
GLOBAL_ID_CONSTANT: Id[ChannelMarker] = Id(12345)
cast_to_another_type = GLOBAL_ID_CONSTANT.cast(MessageMarker)

And with newtypes:

MessageId = NewType("MessageId", int)
RoleId = NewType("RoleId", int)
# etc.

GLOBAL_ID_CONSTANT = RoleId(12345)
cast_to_another_type = MessageId(GLOBAL_ID_CONSTANT)
karmic aurora
#

Rust is able to infer the type in 99% of cases so it's usually much shorter with marker types. We actually used newtypes, but switched as the lack of inheritance caused code duplication for each type (though this can be mitigated with macros)

spare viper
#

I see

#

Thank you for your input!

silent marsh
#

i think it's happening lol

#

they are finally merging the hyper-rustls PR

wild marlin
silent marsh
#

now we wait for a release 🙄

lean geyser
silent marsh
#

they seem to be somewhat detached from hyper too

#

I'm doing my best to help them get this done quickly

lean geyser
#

probably because its a huge HTTP codebase that isn't actually that related to TLS

lean geyser
silent marsh
#

I'm aware, I just think some of the processes they are trying to enforce are a bit silly when activity is so low on that particular repo

#

Having 3 people approve release notes when only 2 of them actually committed to the repo in somewhat recent times seems off

lean geyser
#

3 seems way too high, yeah.

silent marsh
wild marlin
jaunty jewel
#

Nice

silent marsh
#

That CI failure is so wild though

wild marlin
#

I have like no idea why that happens it is a very strange one

silent marsh
#

just a nightly change that improved detection I assume

wild marlin
#

Ahh is that one run on nightly?

silent marsh
#

Yeah I think because of doc_auto_cfg

#

@wild marlin can you test the hyper update PR with native-tls for me?

#

I just can't figure out why cargo build --example XXX --no-default-features won't disable the default features used

#

Oh I'm dumb

#

Nevermind

#

nah editing the Cargo.toml has no effect either

#

it's so weird

wild marlin
#

I think we turned off the coverage because it did not work well

silent marsh
#

what on earth is this

wild marlin
#

Looks like a bad merge made by some dum person vikingblobsved

silent marsh
#

i think your merge added that?

#

yeah

#

do you want to fix it and push it to next or do you want me to do it?

#

i was about to merge main into next

#

and that showed up in the conflict XD

wild marlin
#

You can just do it if you are at it anyway

silent marsh
#

okay

#

seems like it was only the comment

#

the merge itself was fine, i was a bit scared whether it wasn't converted to return errors at finalization

silent marsh
#

and after that I have one more quick PR

wild marlin
#

It only does http1 now?

silent marsh
#

only does http2 now

wild marlin
#

Can you not use both http1 and http2 at the same time anymore?

silent marsh
#

http1 was never actually used

wild marlin
#

I know discord have in the past disabled http2 because of DDOS attacks so I know it was used

silent marsh
#

that seems wrong

#

there is no real point in that for mitigating DDOS to my knowledge

#

anyways HTTP2 is supported and will always be chosen if offered by the server (cloudflare)

wild marlin
#

There are some special attacks that only works on http2

silent marsh
#

which are likely fixed by now then if they re-enabled them

wild marlin
#

Yeah, but that is top say that we should maybe not rely on it always being available

silent marsh
#

i do think that we can and should do that

#

it's definitely been available 100% of the time in the last years as far as i'm aware

#

and cloudflare strongly prefers HTTP2

#

it's the superior protocol in many ways

wild marlin
#

It was turned off at least at the time 18.10.2023 03:35

silent marsh
#

and where is that info from

#

i would assume they make blog posts about this kind of stuff

wild marlin
#

A principal engineer at Discord

#

It broke one of the elixir libs because they relied on http2 being availible

eternal owl
#

We have disabled http2 in the past

wild marlin
#

New version with http1.1 and http2 always looks good, we can look into if we want features for it in the future. (and the addition of http3 when hyper can that I guess)

silent marsh
#

h3 in hyper will take a long time I think

#

Their implementation of h3 is very lackluster and buggy

wild marlin
#

We can always just try and incorperate one of the other implementations as I did in the past, though then it will probably be http3-only if that is enabled.

silent marsh
#

if we can get that PR merged I'll open another one to rename the native feature to native-tls and that's all of the breaking changes we can do for now

#

I'll cut a new release as soon as those two things are done

#

@karmic aurora as far as I remember you had some concerns about releasing the gateway in next as is, is that still the case? I would like for it to be reworked with a stream implementation but I think that's out of scope for a new release at the moment

#

As long as it's not worse than the implementation in main I'm fine with releasing it

#

alternatively I could take a stab at quickly picking up your WIP branch and finalizing it

karmic aurora
#

I have continued developing my patch and it's shaping up nicely (it's close to finished). I'll post another status update later today

karmic aurora
#

The logic is now fully done (barring any bugs) and I'm cleaning up the code

silent marsh
#

oh that also reminds me #2179 needs reviews

karmic aurora
#

(Currently draft) PR up: #2309 with complete API changes (TODO: docs & examples)

silent marsh
#

so the only PRs I want to get in for a new release are:
#2308 (native feature rename)
#2309 (Stream impl for Shard)
#2179 (generic cache)

#

the other stuff can be done later and are mostly not breaking changes

karmic aurora
#

Should we do a RC release first?

silent marsh
#

yeah I would actually prefer that

#

once those 3 PRs are in we have a lot of internal changes

#

might be worthwhile to get some more testing of the gateway by releasing rc releases

karmic aurora
#

#2309 grew a little large…

image.png
wild marlin
#

@autumn sequoia Do you have any suggestions how to name them, I think it is fine to put the json in a seperate feature

silent marsh
#

to be completely honest I don't like having the deserialisation in util

#

it's a single line of code that the user can just write themselves

#

if we were to add this we'd have to support simd-json via a feature flag and so on

autumn sequoia
#

I'd be good with deleting it, the main value of the PR is definitely in check_signature()

silent marsh
#

I'm already not too big of a fan of the signature validation in itself but since it's util... I guess we can add these wrappers there

autumn sequoia
#

hm, why aren't you a fan of the signature validation?

silent marsh
#

Abstracting over ~5-10 lines of code that users can write themselves feels wrong to me, especially if it means writing a 120 line long wrapper

wild marlin
#

I think it fits fine into util, because util is exactly for those 10 lines of code everyone otherwise writes again

silent marsh
#

I don't think it's gonna be that massive of a difference compared to using dalek directly

autumn sequoia
#

I won't lie, they're a very rough 5-10 lines to write

silent marsh
#

You'll still have to do error handling and so on, which is the majority of that code you'll be writing

wild marlin
#

Maybe you should update the example as well which currently uses code I wrote around the time it got released back in the day

autumn sequoia
#

you know

#

I didn't even know that example was there

wild marlin
silent marsh
#

The code and toml could use some formatting too but I was gonna mention that in my review

autumn sequoia
#

well it didn't pass the cargo fmt test in CI, so I was gonna push a commit that did that in a bit

#

...okay, cargo fmt didn't make much of a change at all

#

"delete this one empty line" doesn't seem like the goal here ferrisPacman

eternal owl
#

My unsolicited take is that signature validation is a documented operation over headers from the API that most applications need to do, so it doesn't seem unreasonable to support. Otherwise, we'd have people write the few lines to make a request to POST /channels/:id/messages with a content body. Generally we have historically aimed to support what the documentation explicitly writes, so I'm not certain this should be an exception just because it takes as many lines of code to write as other specific implementations do

silent marsh
wild marlin
#

I like to have it in not just because of that, but also because it does not mean that every user have to do research into what crates they should be using and so on.

silent marsh
#

they'll still need to grab the header values and collect the body, we're far away from being an opinionated framework

#

if that were the case we'd provide methods for the types in http

#

as long as this is in util it's fine

#

it's also not too complex of a thing to write that someone who wants to use e.g. a different crypto crate wouldn't be able to write it themselves

autumn sequoia
#

(gonna poof for a bit, but I'm gonna go through the existing CI failures in like an hour and fix them, don't bother approving more CI runs until then 🥴)

autumn sequoia
#

(Mmkay, I'm back.)

#

...can I #[allow(dead_code)] on FromHexError? Its field is there solely so the derived Debug impl delecates into it.

silent marsh
#

you can add an implementation of std::error::Error

#

via Error::source you can expose it

autumn sequoia
#

That would make perfect sense if hex::FromHexError implemented std::error::Error ferrisButGrimacing

#

though the reason I wrapped it that way at all is that hex is pre-1.0, so I avoided putting it in public API

#

🥴

silent marsh
autumn sequoia
# 
error[E0277]: the trait bound `hex::FromHexError: std::error::Error` is not satisfied
  --> twilight-util/src/signature_validation.rs:23:14
   |
23 |         Some(&self.0)
   |              ^^^^^^^ the trait `std::error::Error` is not implemented for `hex::FromHexError`
   |
   = note: required for the cast from `&hex::FromHexError` to `&dyn std::error::Error`
silent marsh
#

std feature is on?

#

for hex?

autumn sequoia
#

ah, probably no

#

✨I Did Not Remember That Existed✨

silent marsh
#

yeah it's quite hidden, I just click view source on the docs.rs page to check which features I need to enable for this stuff

#

people should really use doc-auto-cfg more

silent marsh
#

we need one review each on #2309 and #2179 and then we can do a release candidate of 0.16

#

cc @primal torrent you wanted to take a look at #2179 again

wild marlin
#

I will probably skip #2179 if possible as I don't really know much about the cache system as that is what I have worked the least with, but I will have a look at #2309.

silent marsh
#

I'll be honest I'm not happy with the amount of unwrapping in #2309 but it seems like improving on that isn't really possible right now. Since it won't change functionality I approved the changes for now but I'll probably be following up with a future PR to attempt to reduce that (not urgent now)

wild marlin
#

I can say that I am not a fan of twilight_gateway::deserialize_wanted tbh I much rather have that nicely put into a wrapper type.

silent marsh
#

We could make an EventStreamExt that adds .deserialize(EventTypeFlags) to a Stream<Item = Result<Message, Error>> or something similar

wild marlin
#

The only reason I can see for doing it this way would be to move this twilight_gateway::deserialize_wanted function to the model crate such that we could upgrade them seperately.

karmic aurora
#

The problem with wrapping streams is that is's really easy to trip up the borrow checker into not accepting access of the shard as it's "moved".

#

And adding a stream type of our own (instead of using StreamExt::map doesn't really solve the issue)

wild marlin
#

No but when every user have to add the same 2-3 lines of code that they surely will just copy I think we could as well have our own extension trait.

silent marsh
#

Yeah that's why I think a trait is better

#

Wrapper types on streams are meh

#

And would allow chaining with existing StreamExt methods

wild marlin
#

I would be nice to be able to move that deserialize function into its own crate whatever it is twilight-model or twilight-gateway-model so we could try and do semantic versioning.

wild marlin