#Development Chat

1 messages · Page 3 of 1

silent marsh
#

That's a very interesting idea

#

The only issue with having it in model would be that model would conditionally depend on futures-core

wild marlin
#

@ocean nest I think I will have a look at the Docker stuff and see if I can get it running again.

silent marsh
#

(If we go the trait route)

wild marlin
#

Yeah that was one of the reasons I think a twilight-gateway-model crate or something could work

silent marsh
#

And a separate crate is... welp

#

the workspace keeps on expanding

wild marlin
#

Though it would have to be updated in lockstep with twilight-model

#

At least major updates

silent marsh
#

eh

#

the issue is

#

with the extension trait you'll still depend on gateway for the message type

#

and we don't want circular dependencies

#

model can't depend on gateway

wild marlin
#

Argh you are right, hmm

silent marsh
#

we could move the message type to the new crate too

#

having it in model is definitely wrong

silent marsh
#

gateway would transitively depend on model again

#

:^)

wild marlin
#

twilight-gateway-types <- twilight-gateway-model -> twilight-model
^
|
twilight-gateway

This seems wrong as well

#

wait that is wrong

silent marsh
#

I'm not too sure if this is worth having to add two new crates

wild marlin
#

Yeah that seems messy

silent marsh
#

@wild marlin the docker failures are because recent nightlies removed -Z gcc-ld

wild marlin
#

ah

silent marsh
#

I'll dig a bit

#

and fix it

wild marlin
#

I can fix it as well, been doing docker musl aarch stuff at work anyway

silent marsh
#

should be as easy as using a different option I guess

#

oh

#

tbh we can just use -fuse-ld

#

exists since GCC 9

karmic aurora
#

I got an MVP extension trait working (taking &mut self to avoid borrow checker issues)

#

Also, the original reason for parse being public was that Event::GatewayClose didn't exist and it felt cruel for users to have to re-implement it if they needed access to close codes. With Event::GatewayClose and a Stream extension trait (Result<Message, Err> -> Result<Option<Event>, Err>) I don't think it needs to remain public.

ocean nest
silent marsh
#

I'll revisit this later but I think this is the only way I'm able to do exactly this

#

I need the raw message and the parsed event later

karmic aurora
#

The stream extension is generic so you could use something like once to deserialize

silent marsh
#

I think for low-level usage, deserialize_wanted should just remain public

karmic aurora
#

If it's supposed to be low-level then its signature ought to be String -> Result<Option<GatewayEvent>, Err> instead of Result<Message, Err> -> Result<Option<Event>, Err>

silent marsh
#

for which method? Currently deserialize_wanted takes Message and EventTypeFlags which seems good to me

#

the stream extension should take EventTypeFlags and go from Result<Message, Err> to Result<Option<Event>, Err>, yeah

#

it can just call deserialize_wanted

karmic aurora
#

I meant that parse should be kept as is then and deserialize_wanted should not be kept as it's not as low level

silent marsh
primal torrent
#

i'll try to look at it this weekend. but things have been crazy busy for me

silent marsh
#

TBH this is the sort of thing we could still change after a release candidate

#

if your only remaining concern was the From impls I hope that isn't too much to review

#

And if you don't find time, just let us know and I guess we'll just merge it and see what the feedback to the rc looks like

#

In the end this is mostly just API design, we probably won't get it 100% right initially

primal torrent
#

i think so but i'll try to take a look at it to be sure, it has been a while

karmic aurora
#

I named the stream extension method deserialize but I'm totally open to changing it to something better

silent marsh
#

And matches what it does more precisely

#

or parse_next if we're keeping the parse method for low-level usage

karmic aurora
#

I dislike adding next to it since it doesn't poll the stream for you (though that might be an even better behaviour?). Currently it's like filter_map except that it takes &mut self instead of self

#

I.e. instead of returning a type implementing Stream it could return a type implementing Future (like StreamExt::next)

#

then we might even call it next_event lol

wild marlin
#

I think it makes sense to just call it events

#

Does the extension return a struct?

wild marlin
#

I think we should rename the returned struct to Events and the method events that at least to me seems like how others are naming a streamExt

#

Also maybe we should call it something else than StreamExt maybe EventStreamExt? or what do you think+

silent marsh
#

It is so weird to call it events

#

But maybe I'm wrong, I'll have to read the source again

#

events as a name would make sense if it creates a new stream

#

but not if it polls and deserializes

karmic aurora
wild marlin
#

The tokio StreamExt docs does not agree that is a good idea o

silent marsh
wild marlin
karmic aurora
#

that's specifically referring to futures_util::StreamExt which collides with some methods of tokio_stream::StreamExt (like tokio_stream::StreamExt::next and futures_util::StreamExt::next). But no other stream extension defines a events method so it should be fine for us

karmic aurora
wild marlin
#

Can you access the stream inside of the event loop?

#

is it not exclusivly held by the EventStream?

karmic aurora
#

You can

silent marsh
#

I think the stream is dropped immediately

#

And recreated on the new iteration

#

But I'll check the PR again now to see what you did

karmic aurora
#

that's right

wild marlin
#

Ahh makes sense

karmic aurora
# 
let mut stream = shard.events(EventTypeFlags::all());
while let Some(item) = stream.next().await {
  // borrow check error
  // printlin!("{:?}", shard.id());
}
silent marsh
#

yeah since we create a new stream in the loop and call .next on it, it works

#

this is actually really neat

#

It wasn't what I originally envisioned but it's also not bad

#

I wanted an extension that added a method to a stream that yields messages which functions like next but also calls parse

#

creating a new stream is just as viable

#

although I believe a new stream is only really superior if you can make use of the stream by not re-creating it

#

stream.events(...).next() vs stream.next_event(...), basically

karmic aurora
#

most stream methods are free, but not all (like timeout(), which does a syscall + registers a tokio timer) so to avoid footguns it might be better to not return a stream

silent marsh
#

With next_event(...) we'd just have to return a type that implements Future

#

should be very straightforward

#

delegate to poll_next and call parse

karmic aurora
#

In fact, we could even bump our msrv and have it be an async fn directly

#

or just make it a direct method on Shard without a StreamExt trait at all

silent marsh
#

1.75 is a tad bit too recent for my taste as well

silent marsh
#

not that it would make a lot of sense but I prefer at least having the option

karmic aurora
#

The problem is that filter and pretty much all stream methods take self such that it's tricky or impossible to access the shard afterwards

silent marsh
#

I don't see why you'd filter away raw websocket messages anyways, next_event would do that for you

#

but having the method on the shard directly just feels wrong

karmic aurora
#

@wild marlin what do you think

wild marlin
#

Tbh I like the next_event method the most as that is the simplest way, and we could make the raw stream accessible if users want it. If that method live in the trait or the stream itself I am not sure

#

I think both could work, and if we put it in the trait it allows some more experiments with where it lives in the future

karmic aurora
#

opted for StreamExt::next_event

silent marsh
#

Nice, let's try to get it merged this weekend

wild marlin
#

@karmic aurora Why was the examples/gateway-parallel.rs example removed?

#

I think we should have a least one rather simple example using multiple shards, at the moment it is only the reshard example

#

Made a review and apart from that it looks good.

silent marsh
wild marlin
#

Then I think we should probably just duplicate it

#

I like having examples in the folder

#

But if you others have a differing opinion then it is fine

karmic aurora
#

We don't duplicate the current README examples in /examples/

wild marlin
#

I just think it is a bit odd that the examples we have is not how most want to use it

wild marlin
#

But yeah feel free to merge

wild marlin
#

👏

silent marsh
#

i rebased #2179 on next again

silent marsh
# 
    = note: the following trait bounds were not satisfied:
            `twilight_gateway::Shard<Arc<InMemoryQueue>>: futures_util::Stream`
            which is required by `twilight_gateway::Shard<Arc<InMemoryQueue>>: futures_util::StreamExt`
#

what did I miss

#

twilight_gateway::Shard<InMemoryQueue> implements it

wild marlin
#

Versions?

silent marsh
#

i use git deps

#

for all crates

#

well that is problematic

# 
error[E0277]: the trait bound `Arc<InMemoryQueue>: Queue` is not satisfied
   --> twilight-gateway/src/shard.rs:969:22
    |
969 |     assert_impl_all!(Shard<Arc<InMemoryQueue>>: Stream);
    |                      ^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `Queue` is not implemented for `Arc<InMemoryQueue>`
    |
    = help: the following other types implement trait `Queue`:
              InMemoryQueue
              &T
#

ah nvm InMemoryQueue is clone now

wild marlin
#

@silent marsh its annoying to test tokio-websockets locally, it seems that the non msrv thing might be broken

#

And sorry for the review after you merged it blobsved

silent marsh
jaunty jewel
#

Hmmm

karmic aurora
#

us being behind the latest api changes I guess. if anyone wants to bring us up to date, we could reimburse you for your work

jaunty jewel
#

I'll try to find some time to work on the lib

#

Indeed, I've been very busy with school stuff =.=

jaunty jewel
#

We've been lagging behind on PRs.. well

#

^^ should this be merged

silent marsh
#

maaaaybe I can do it this evening

jaunty jewel
wild marlin
#

Ahh I should read the rest of the chat

jaunty jewel
#

understood

karmic aurora
#

ahash 0.8.8 (via simd-json -> halfbrown -> hasbrown) bumped its MSRV to 1.72 and broke CI

jaunty jewel
#

Do we want to bump our MSRV just because of that

silent marsh
#

creates confusion for users

jaunty jewel
#

Agreed

silent marsh
#

and annoying to validate in CI

#

1.72 sounds like a reasonably old version

jaunty jewel
#

currently nightly is 1.78, so subtract 2

#

current stable is 1.76

silent marsh
#

Generally speaking, 4-5 releases old is fine

jaunty jewel
#

yeah 4 releases back

silent marsh
#

That's about a little more than half a year

jaunty jewel
#

I guess we can push ahead and bump the MSRV to 1.72

silent marsh
#

yeah feel free to PR it

jaunty jewel
#

I'll get to it today after I finish up the final steps to rust-lang/rust 120840 :v

silent marsh
#

@wild marlin @karmic aurora @jaunty jewel I've updated #2179 with Erk's idea (basically collecting the trait bounds into a supertrait). This makes the code more readable, it's essentially just an alias for the generics. I've had some talk with Erk about the naming of these types, but overall I think what we have now is fine and in line with the names of the other new traits introduced.

jaunty jewel
#

And I'll PR the MSRV bump

silent marsh
#

seems like ahash is backing away from the MSRV change but no new release yet

jaunty jewel
#

oh well

#

I guess we don't have to do the MSRV bump for now

silent marsh
#

i think we should still bump it

#

having headroom is always nice and it allows for some refactorings

#

since we stay on the same minor release for quite a while now apparently, we should make some space for newer language features to be used in PRs

jaunty jewel
#

well indeed

jaunty jewel
silent marsh
#

don't forget to run cargo clippy --fix and update the badges

jaunty jewel
wild marlin
#

hmm for some reason github is broken when I try to approve the changes, but maybe it is just the bad train internet

#

Now it went through, in the 3rd attempt

silent marsh
#

I can merge stuff and do releases today if someone approves #2316 to get CI working and then re-acks #2179 (Erk approved it but the other two approvals are for an old version of the PR)

#

Actually I'll PR the MSRV bump really quick after #2316 as well... I just want to be done xD

#

But without CI there is no point in merging anything

#

Hmm now that I'm looking at it, I assume vilgot wants #2315 as well

karmic aurora
#

My understanding is that the intra-bucket order doesn't matter but is it really the case that each rate_limit_key must be the minimum shard? I've never seen any library, including ours, handling this. I've written a patch that switches to priority queues, but I'm unsure whether or not to PR it.

#

I would assume this would trigger bugs during mass-disconnects…

karmic aurora
silent marsh
#

Yeah that was what I was gonna do

#

Gonne merge main into next now to fix CI

silent marsh
#

I'll address your review now

silent marsh
karmic aurora
#

damn you merged to next before me

#

though I think you messed up the conflict

#

nvm

silent marsh
# 
type annotations needed for `InMemoryCache<CacheModels>`
cannot satisfy `_: CacheableModels`
the trait `CacheableModels` is implemented for `DefaultCacheModels`rustcClick for full compiler diagnostic

@karmic aurora btw compiler still needs it

#

that's because new is defined in the impl<CacheModels: CacheableModels> InMemoryCache<CacheModels> section

#

but I don't want to get rid of the constructor for everyone with custom types

karmic aurora
#

til

silent marsh
#

we could add a new new_default() instead

#

but i think that doesn't really make up for it

#

PR is rebased, waiting for CI then I'll merge

silent marsh
silent marsh
#

I'll head to bed, once we get another review on #2315 I will do RCs tomorrow

jaunty jewel
jaunty jewel
silent marsh
#

I'll do the releases now

#

Gonna have some fun writing changelogs ^~^

#

lol our changelog script broke

#

newer cargo versions changed output of cargo metadata --no-deps --format-version 1

wild marlin
silent marsh
#

Yeah that seems to be the JSON format only

#

But the file paths look different

#

I'll look at it deeper when I'm done with releases

wild marlin
#

ahh makes sense I guess maybe

silent marsh
#

For now cargo +1.64 metadata works around it for me xD

#

uhh seems like cargo-release is stuck halfway through

#

the dry run worked but now it hangs at 100% cpu

# 
   Packaging twilight-gateway-queue v0.16.0-rc.1 (/home/jens/twilight/twilight-gateway-queue)
   Verifying twilight-gateway-queue v0.16.0-rc.1 (/home/jens/twilight/twilight-gateway-queue)
    Updating crates.io index
error: failed to verify package tarball

Caused by:
  failed to select a version for the requirement `twilight-http = "^0.16.0-rc.1"`
  candidate versions found which didn't match: 0.15.4, 0.15.3, 0.15.2, ...
  location searched: crates.io index
  required by package `twilight-gateway-queue v0.16.0-rc.1 (/home/jens/twilight/target/package/twilight-gateway-queue-0.16.0-rc.1)`
  if you are looking for the prerelease package it needs to be specified explicitly
      twilight-http = { version = "0.15.0-rc.1" }
  perhaps a crate was updated and forgotten to be re-vendored?
#

huh

#

it just decided to publish gateway-queue before http

#

i think cargo-release messed up badly, idk how

#

AAAAAAAAAAAAAAAAA

#

and cargo-release did not change the version in the CHANGELOG.md files

#

or does it not do that for pre-releases

#

mmh seems so

#

@wild marlin @karmic aurora how do we want to do this? Releases are out on crates.io.
Should I publish release notes in GitHub release and mark it as a pre-release? (I'd rather put the full changelog in a stable GitHub release later and not do one for now)
Should I do an announcement in #announcements? (I think we should)
Should we write a book entry for a pre-release? Seems weird

#

I think the best thing to do is having me write a book entry now, PR it but don't merge it until we release a stable 0.16

wild marlin
silent marsh
#

I'm writing one as we speak :)

wild marlin
#

And yeah I agree we should do a #announcements

silent marsh
#

Will do the announcement once i have a book PR up

wild marlin
#

Maybe we should merge it as well so we can point to the page

silent marsh
#

hmm yeah sure

silent marsh
#

i hate writing changelogs, i'm never sure what to mention

#

if i mention every single commit i'm never gonna finish

karmic aurora
#

I think it's best to consider it a sort of upgrade guide

silent marsh
#

That's the part I wrote so far

#

The Discord API catchup part is the one that bothers me

wild marlin
#

I will probably do a bit of catchup this weekend I think we are a bit behind, though that is probably more for 0.16.½

silent marsh
#

once the book stuff is merged i'll announce the release

wild marlin
#

Otherwise it looks good

silent marsh
#

the 0.15 one didn't have all imports, but I'll add the trait

#

Added!

silent marsh
#

@karmic aurora i'm hopping on a train now so if there's anything else you want reordered, feel free to just do it :)

silent marsh
#

Also, I'd aim for a stable release next weekend unless we hear of any breaking bugs

jaunty jewel
silent marsh
#

So I did get an indirect bug report via my gateway-proxy repo

#

This is a very interesting catch

#

I'm not 100% what the right decision is here API-wise

#

Either tokio-websockets should only use the configured connector if the scheme is wss, or twilight should set the connector to plain if the proxy URL is set

#

I think there's arguments for both

wild marlin
#

or both

silent marsh
#

I personally think that setting a connector should equal use this, no matter what

wild marlin
#

Then I think doing both would be a good way forward.

silent marsh
#

The scheme guessing should be best-effort in case a connector hasn't been set

wild marlin
#

I am not sure I agree with that since scheme is how you decide if you should use tls in the first place

#

if it is not wss tls should not be used

silent marsh
#

Hmmmm

#

Yeah fair I guess

#

I need to read up on the RFCs for this

wild marlin
silent marsh
#

Seems like you're right then

#

I'm mainly worried about nonstandard protocols that build on websockets and may use a different scheme

wild marlin
#

I am not even sure other libraries will say ok if the protocol is not ws/wss

silent marsh
#

But I guess connect_on is the solution for that...

wild marlin
#

Yeah that would make sense, if you allows "bring your own socket" people can do whatever

silent marsh
#

I personally think twilight should remain untouched

wild marlin
#

Though even there you need to set a uri

#

Yeah I think that would make the most sense

silent marsh
#

It's entirely possible to run the gateway proxy behind nginx or so with TLS

#

All that's needed is a small change to tokio-websockets to use plaintext with ws scheme no matter if a connector is configured

wild marlin
silent marsh
#

If they are, I'll have to drop an 0.6 release

#

They shouldn't be, but 0.5.1 was technically a breaking release even though I thought it wasn't >.<

#

I saw hyper-rustls had a CI check for semver compatibility

#

Will try to set that up

karmic aurora
#

we don't need to bump tokio-websockets if we call Client::connect_on directly

wild marlin
#

I think a better solution is to fix the bug in tokio-websockets

karmic aurora
silent marsh
#

But that should be backwards compatible

karmic aurora
#

I see

wild marlin
#

Hmm I am not sure if this is the tws patch or my stuff acting up

#

@tame nimbus have you seen that error?

tame nimbus
wild marlin
#

Then it is probably my codes fault

#

hmm reallyu strange I just tried when I got home and now it works, guess it might just have been a issue with the train internet or something

tame nimbus
#

strange

silent marsh
wild marlin
#

@silent marsh I just found a issue with tokio-websockets, you can create invalid strings:

use bytes::Bytes;

use tokio_websockets::{Payload, Message};

fn main() {
    let arr = Bytes::from(vec![0,0,0]);
    let bla = Payload::from(arr);
    let msg = Message::text(bla);
    let ub = msg.as_text().unwrap();
    println!("{} | {:?}", ub, ub.as_bytes());
}
#

It is just assumed that any frame with the text opcode only contains valid text

silent marsh
#

I would hate for Message::text to perform utf8 validation

wild marlin
silent marsh
#

maybe we can extend payload with a flag whether it's valid utf8

silent marsh
#

That doesn't solve anything

wild marlin
#

oh yeah wrong one, that one and the bytes impl, but then it is probably better to find another solution

#

You do UTF8 validation on the incoming messages right?

silent marsh
#

yes

#

this is not really an issue tbh

#

the code snippet you sent is the only way to reproduce it, and that's not something you're supposed to do (create a message yourself and then treat it as a received one)

wild marlin
#

Well it is a way to do UB, so it is not a issue with the usage, but it breaks the invariants of std string.

#

Yeah it is not something that you will run into by accident

silent marsh
#

The entire idea behind having Message::text take a payload is to allow serializing e.g. JSON into a BytesMut and then using that to avoid allocations

#

Which is why I don't want to change that API

#

I'd just say add a documentation comment for Message::text saying payload must be valid UTF-8 and call it a day

#

Performing validation of the payload is useless overhead

wild marlin
#

I disagree

#

Then mark it as unsafe. I don't think it is a good idea to allow UB.

silent marsh
#

I'm not going to mark that as unsafe either, it doesn't cause UB when the API is used as intended

wild marlin
#

I disagree with that statement very much.

foggy spire
#

allowing users to encounter UB by using safe rust sounds like a bad idea

silent marsh
#

I agree, but marking an API as unsafe that isn't unsafe unless you basically write code that is complete garbage and no-op is not great design either

wild marlin
#

Then we should go back to the drawing board to figure out a way to ensure it.

silent marsh
#

I have an idea

#

adding a bool to Payload called utf8_validated should fix it

wild marlin
#

The UTF8 validation could be done in as_str, but then you do double work in the usual case.

#

Yeah that could work

silent marsh
#

we can set that for incoming messages after they are validated

wild marlin
#

And then do UTF8 validation if you turn it into a string when it is false.

silent marsh
#

and unvalidated gets checked in as_text

#

Ideally I'd want Message::text to perform the check but that's such a ridiculous overhead I'd rather not

#

@wild marlin If you have a moment or two, I'll open two PRs now for the fixes

wild marlin
#

@silent marsh re the first one, tbh I think there should be added a new error that is returned if it not wss or ws instead of just asuming it is plain if it is not wss

silent marsh
#

I think so too but that's breaking

#

Okay screw it, let's just do 0.6 with all the fixes

#

Being afraid of version numbers doesn't help anyone

wild marlin
#

I think together with changing as_text to returning a result it is worth

silent marsh
#

as_text currently already returns an Option

#

ugh

#

Option<Result<&str, Error>> feels weird

wild marlin
#

The other way around makes more sense to me

#

or maybe not

silent marsh
#

not really

#

None is intended for when the message isn't a text message

#

but this API is generally awkward

#

I'd rather panic

silent marsh
#

which is already a red flag

wild marlin
#

Yeah maybe it makes sense to panic.

#

Just getting some food, but I will have a look at the PR's later.

wild marlin
#

Both the pr's look good

silent marsh
#

i'll let vilgot look at the second one just to be sure

#

payload internals are mostly his design idea

#

I think I can probably move utf8_validated to the Payload struct

#

might make the code more readable

silent marsh
wild marlin
#

Just spotted a small thing when I had another look, the //SAFETY comment is outdated

silent marsh
#

which one?

wild marlin
#

"Opcode is Text so the payload is valid UTF-8"

silent marsh
#

ahh

#

I guess that can be completely removed

#

Ah no, I want to keep the safety comments for all unsafe usage

karmic aurora
#

@silent marsh I still don't like the as_text docs.

silent marsh
#

Me neither

#

I have no idea how to improve them though

#

I can totally see how people might misread them

# 
    /// Returns a reference to the message payload as a string if it is a text
    /// message. For received messages, this is zero-cost.
    ///
    /// # Panics
    ///
    /// It is theoretically possible to craft a text message via
    /// [`Message::text`] using a payload with invalid UTF-8. This will not
    /// cause undefined behavior when sending, since the remote end is required
    /// to validate the UTF-8. In the extremely unlikely and undesirable event
    /// that this crafted message's payload is then casted to a string by
    /// calling this method, it will panic to avoid undefined behavior.
#

how about this?

karmic aurora
#

I think less is more, or whatever.

silent marsh
#

I don't really know how to explain it shorter without making it less easy to confuse

#

I guess the second sentence could be removed

# 
    /// It is theoretically possible to craft a text message via
    /// [`Message::text`] using a payload with invalid UTF-8, since that method
    /// avoids validating UTF-8 for performance reasons. In the extremely
    /// unlikely and undesirable event that this crafted message's payload is
    /// then casted to a string by calling this method, it will panic.
#

maybe undesirable is the wrong word here, what i want to express it that this is a massive antipattern and should never be done

# 
    /// It is theoretically possible to craft a text message via
    /// [`Message::text`] using a payload with invalid UTF-8, since that method
    /// avoids validating UTF-8 for performance reasons. If the crafted
    /// message's payload is then casted to a string by calling this method,
    /// it will panic. This edge case indicates flawed logic in caller code.
karmic aurora
#

This turned into quite a bit of text. TL;DR I prefer my exact review suggestion.

For received messages, this is zero-cost.
As a user, I would assume the method to be optimized so pointing it out feels odd. Why would it ever not be zero-cost if possible?
The standard library sometimes annotates conversions as guaranteed O(1), for example, but I don't think that use-case if needed here since there's no alternative to this method (where the standard library would offers specialized alternatives).

This method will panic when the message was created via Message::text with invalid UTF-8. This does not include received messages, they are validated during the decoding stage.
I think the first sentence is enough. The internals of how poll_next creates its Messages is not something users should worry about so the documentation need only mention public constructors. Also let's not forget that the struct level documentation for Message includes
Received messages are always validated prior to dealing with them, so all the type casting methods are either almost or fully zero cost.

It is theoretically possible to craft a text message via Message::text using a payload with invalid UTF-8, since that method avoids validating UTF-8 for performance reasons. If the crafted message's payload is then casted to a string by calling this method, it will panic. This edge case indicates flawed logic in caller code.
Way to complicated. The docs should be very simple and structured so users can skip the verbiage and only focus on the panic conditions.

silent marsh
#

Ah yeah I completely forgot the top-level struct documentation

#

That makes things a lot easier here

#

@karmic aurora Pushed to match your review suggestion

karmic aurora
#

besides yanking 0.5.1 does this also warrants a rustsec entry (essentially a CVE)?

silent marsh
#

i don't think so

#

and neither do i think we should yank 0.5.1

#

the code to cause UB makes no sense

lean geyser
#

not really sure I agree with the "makes no sense" part. If theres code out there that can cause UB from safe code it should be yanked once a replacement is available

silent marsh
#

i don't feel like breaking people's lockfiles over an issue that isn't present in anyone's code

wild marlin
silent marsh
#

I thought yanking completely removed all artifacts of the release from crates.io?

eternal owl
#

Yanking prevents crates from newly depending on the yanked version

silent marsh
#

Ah, then that seems fine

#

I'll yank it once the new release is published

wild marlin
#

Oh sorry @tame nimbus i had not seen you were working on twilight 0.16 as well

silent marsh
#

yikes

#

That is one of the most confusing errors I've seen so far

#

I assume it's because the Resolver trait does not mandate the Future to be Send?

#

I cannot reproduce anything like this locally xD

silent marsh
#

the hell :D

wild marlin
#

The error?

primal torrent
#

a proper test would fail but you could run into it

silent marsh
#

This sucks a bit because I'll have to bump minor version again

karmic aurora
silent marsh
#

Ah oh well i guess you are referring to the dyn Future

#

I didn't want to raise MSRV by that much

#

I'm considering just requiring the return type to be a oneshot::Receiver<Result<SocketAddr, Error>>

karmic aurora
#

Most async trait fn's will have a Send bound for this exact reason, requiring a channel is excessive

#

All tokio interfacing libraries are already plagued by send bounds

karmic aurora
silent marsh
#

I'm not 100% sure. The code as it is now matches what async_trait would require so I went with that to be safe

#

async_trait has a where Self: Send bound

karmic aurora
#

Relaxing the bound is thankfully not a breaking change 😅

silent marsh
#

seems like CI in twilight passes with 0.7 so I can relax now

wild marlin
silent marsh
#

Since that future is then awaited in a tokio task XD

wild marlin
#

Should we touch out a rc.2 of the gateway?

silent marsh
#

I think we should rather just do the release on the weekend, that was the only bug report and it was very minor

wild marlin
#

@silent marsh Do you have time to do it today?

silent marsh
wild marlin
#

Sounds good

silent marsh
#

By the way, there's a new rustls version that switches the default crypto provider to aws-lc-rs, which I'm not particularly fond of

#

The platform compatibility of aws-lc-rs is much worse than ring

wild marlin
#

Argh, fun even more stupid features to add to all our crates.

#

This also mean that any crate that does not remove the default features is just unsupported on freebsd.

silent marsh
#

Yeah we'll have to offer a selection between aws-lc-rs and ring

#

And should probably default to ring...

#

I still don't understand why they did that

#

"it's FIPS compliant" does not strike me as a great argument since that's opt in

wild marlin
#

It's not fips compliment by default

silent marsh
#

that's what I meant by opt in, yeah

wild marlin
#

Ahh yeah, I should have finished that line vikingblobsved

silent marsh
#

from what I've read on their discord platform compatibility is an oversight but they don't wanna go back

wild marlin
#

And soon you also have the Microsoft lib that is fips compliant as well

#

I wonder what happens if I compile rustls on freebsd, does it actually complain about platform stuff

silent marsh
#

from what I've read, this should break BSD and RISCV targets out of the box (?)

wild marlin
#

2 secs let me try on my freebsd vps

silent marsh
#

I guess your FreeBSD and the Power machine are good testing areas xD

#

I wiped my Pi 4 that was running FreeBSD

wild marlin
# 
   Compiling aws-lc-sys v0.13.2
error: failed to run custom build command for `aws-lc-sys v0.13.2`

Caused by:
  process didn't exit successfully: `/home/erk/dev/rustls/target/debug/build/aws-lc-sys-d18c1cf2bfcc51a2/build-script-main` (exit status: 101)
  --- stdout
  cargo:rustc-cfg=use_bindgen_generated

  --- stderr
  Missing dependency: cmake
  thread 'main' panicked at /home/erk/.cargo/registry/src/index.crates.io-6f17d22bba15001f/aws-lc-sys-0.13.2/builder/main.rs:256:34:
  called `Result::unwrap()` on an `Err` value: "Required build dependency is missing. Halting build."
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
warning: build failed, waiting for other jobs to finish...
#

Great error :)

#

Going to be fun to make a pr to every single crate that just writes rustls = "0.23" NotLikeThis

silent marsh
#

:^)

wild marlin
#

lol aws-lc-sys will actually compile on other platforms, the only reason it does not is because they pre-generate the bindgen bindings for some of them.

#

(if it will work on other platforms is still a open question)

# 
  /home/erk/dev/aws-lc-rs/aws-lc-sys/src:
  drwxr-xr-x. 1 erk erk     388  8 mar 09:47 .
  drwxr-xr-x. 1 erk erk     228  8 mar 09:47 ..
  -rw-r--r--. 1 erk erk    2247  8 mar 09:47 lib.rs
  -rw-r--r--. 1 erk erk  927498  8 mar 09:47 linux_x86_64_crypto.rs
  -rw-r--r--. 1 erk erk 1124362  8 mar 09:47 linux_x86_64_crypto_ssl.rs
  -rw-r--r--. 1 erk erk  925324  8 mar 09:47 linux_x86_crypto.rs
  -rw-r--r--. 1 erk erk 1122179  8 mar 09:47 linux_x86_crypto_ssl.rs
  -rw-r--r--. 1 erk erk  925309  8 mar 09:47 linux_aarch64_crypto.rs
  -rw-r--r--. 1 erk erk 1122173  8 mar 09:47 linux_aarch64_crypto_ssl.rs
  -rw-r--r--. 1 erk erk  928864  8 mar 09:47 macos_x86_64_crypto.rs
  -rw-r--r--. 1 erk erk 1126349  8 mar 09:47 macos_x86_64_crypto_ssl.rs
#

Which just makes it so much worse, because it does not even support windows...

#

Or wait it says it does support windows???

#

Ahh for that you need to build the bindings yourself.

silent marsh
#

it needs nasm on windows

wild marlin
#

Yeah, the build tools, cmake, nasm and llvm.

#

*a old build tools, the newest one is unsupported.

silent marsh
#

I think the big shitstorm hasn't started yet because tokio-rustls hasn't had a release yet

#

But I imagine there'll be a lot of complaints once it's out

wild marlin
#

Yeah one of the most common complaints with ring was poor support, but this seems worse.

#

Ring has done a lot to work across a lot of platforms now

silent marsh
#

I can't wait for the day when RustCrypto is usable

lean geyser
#

will be nice yeah

silent marsh
#

It's always seemed silly to me that the pure rust TLS implementation had a C crypto component

lean geyser
#

I dont think twilight should expose awc-lc-rs. its not useful to end users of the library afaict

#

No one needs FIPS compliance talking to Discord on their Linux server

silent marsh
#

i disagree, aws-lc-rs generally is faster than ring and therefore a viable option, just not by default

lean geyser
#

do you have the benchmark link onhand?

wild marlin
#

But what should we expose? should we just add 2 more features to each of our crates that may use rustls?

silent marsh
#

yeah there is a bot in the rustls repo that does automated benchmarks

#

one sec

lean geyser
#

both are using the same C code underneath, boringSSL :P

#

so im curious to see

#

Note that RustCrypto performance is generally inferior than ring, but in exchange you got a pure Rust implementation that theoretically compiles everywhere Rust was ported to. In our case, we need to have std but foundational support for future no_std expansion is already here.

This package is still in its very early phase, so until we think the code is okay for general public use, this won't be published to crates.io anytime soon.

silent marsh
#

that's how all crates that depend on rustls will have to do it

lean geyser
#

speaking of tls stuff I guess, is twilight going to support the new rustls certificate library?

wild marlin
#

And then if symcrypt is added to upstream, we should add another feature :)

wild marlin
lean geyser
#

platform verifier or something

lean geyser
#

i think there will come a point where twilight just has to say "no we arent going to support x TLS feature"

#

its pretty high up in people's dependency stacks and increases cognitive overhead

wild marlin
#

The alternative is doing like songbird is doing and tell people that they have to add feature to rustls themselves, but that is also error prone and adds a new place where things can go wrong.

silent marsh
wild marlin
#

(songbird does that not with rustls, but with symphonia)

silent marsh
#

I imagine there's a better side by side comparison somewhere

lean geyser
silent marsh
#

They rendered them in the repo but only on PRs

lean geyser
#

man this website is slow as hell

silent marsh
#

But that one has metrics for aws-lc-rs and ring and renders them nicely

silent marsh
#

the new hyper-rustls release will have support for it

#

and I think it's pretty neat, should be the new default in my opinion

wild marlin
#

I think we should cut twilight with the current version and then delay the decision to update it to after we have published 0.16 vikingblobsved

#

(current as in the current in twilight)

lean geyser
silent marsh
wild marlin
#

tbh I am the strange person that uses native-tls so it does not really matter that much for me.

lean geyser
#

openssl is a strange choice these days, yes

silent marsh
#

see the bonus 1 section

#

aws-lc-rs is anywhere from 6% slower to 62% faster than ring

wild marlin
#

And it did not use to work at all on odd platforms which was the reason I started to use native-tls.

#

Like now it works on big endian and everything

lean geyser
#

yeah fair

silent marsh
#

It does now

wild marlin
#

Is that not the one that is linux only

silent marsh
#

this crate is compatible with rustls ever since they exposed secret extraction

wild marlin
#

yeah it is

lean geyser
#

i personally know my own HTTP servers will not be exposed to enough traffic that kTLS matters

#

so the benefit of keeping more in Rust is nice

wild marlin
#

I should try to make rustls work with ktls on freebsd

#

I think last I looked at it I stopped because I had to do a bunch of libc prs to make it work without a bunch of custom stuff.

wild marlin
wild marlin
ocean nest
#

yay checks passed so id assume it works

karmic aurora
wild marlin
#

@silent marsh (and @ocean nest) i will probably remove the build-std stuff, as it seems to give a bunch of errors with aarch64

silent marsh
#

build-std definitely works on aarch64

#

Could you share the errors please?

wild marlin
#

There are some issues referencing this in the rust repo and in the compiler_builtins repo

silent marsh
#

That's fixable by linking in libgcc

#

rustup target add aarch64-unknown-linux-musl will download a libgcc for the target

#

I've never gotten it to work with the compiler_rt stuff, but libgcc works fine

wild marlin
#

Okay now it works

wild marlin
wild marlin
silent marsh
#

there should be a new mapping and generic

wild marlin
#

Or should we add a guild_events global thing?

#

I guess that is what you want.

silent marsh
#

And a new resource type alongside as well, yep

wild marlin
#

@silent marsh Should be done now.

#

I also went ahead and integrated with the other scheduled events events

#

Which might be a bit outside of the scope but I was there and it would be strange if it was not included.

silent marsh
#

@wild marlin I have a couple of issues with that new dockerfile but not much time for leaving an actual review. My main problem with it is that you're hardcoding cross compilation targets, while the old file allowed for cross compiling to any target

wild marlin
#

No it did not

silent marsh
#

It did

#

You just had to specify the targets via --build-arg

#

This new file hardcodes the arm64 libgcc installation and supported targets

wild marlin
#

Ahh in that way

#

Yeah it did work with musl targets, though none of them was tested.

#

The reason I have to hardcode the libgcc is a bug in rust, which is rather annoying

silent marsh
#

back when I wrote the file I built 32-bit ARM and s390x just to see that it works

#

and I think it's important to have a dockerfile that allows for self-compiling to any target musl/alpine supports

#

otherwise this is internal only and not usable by people who need to build it themselves

wild marlin
#

The issue is not that, the issue is that you need to use gcc to build your own stdlib

#

(on arm at least)

#

If I removed that it would have as good support as the last one.

#

(and did a bit of fiddeling with some env variables)

silent marsh
#

you're still hardcoding the targets in .cargo/config.toml though

wild marlin
#

Yeah, but that can be changed to be as before

silent marsh
wild marlin
#

Yeah, if you want I can change it back to musl.cc if you think that is the better way to cross compile.

silent marsh
#

considering it allows for more supported targets I think it is

wild marlin
#

I am just not a big fan of musl.cc since it is like 2 years out of date

silent marsh
#

I was messing around with using LLVM and compiler-rt

#

btw this is the cause

#

but that didn't work for me

wild marlin
#

Yeah those functions are unimplemented

silent marsh
#

yeah I was telling it to use the C fallback

wild marlin
#

ahh

silent marsh
#

so that I could use compiler-rt

#

Instead of libgcc

wild marlin
#

Doing that is rather messy

#

I tried for a bit as well

silent marsh
#

it is the better way though

#

LLVM over GCC has a long list of advantages

#

linking in libgcc alone has some licensing implications afair

silent marsh
wild marlin
silent marsh
#

yeah I had that set up properly and building fine

#

but it still gave me the symbol errors anyways

#

was weird

#

I should try it again

#

iirc my issue was that it then built compiler-builtins twice

#

and probably linked in the wrong one

wild marlin
lean geyser
#

what a cluster of a crate

wild marlin
#

@silent marsh Do you plan of updating rustls in Twilight before we release 0.16?

silent marsh
#

Yes

#

tokio-websockets is basically ready, I hope hyper-rustls won't take too long

#

Oh, hyper-rustls is ready already

#

Even better then

wild marlin
#

Oh yeah was jsut about to say that

#

And it seems the tracing thing have been fixed as well so I think your patch to tokio-websockets is ready, let me know if you want a second review of it or something

silent marsh
#

The tracing thing was fixed by bumping tokio-util to 0.7.3

#

(And dropping the very ehhh redundant hyper support)

wild marlin
#

Maybe you should also not that in the commit that ends up on the main branch

silent marsh
#

I was going to add a commit to export make_key and then note that as the replacement

wild marlin
#

And maybe the lavalink v4 stuff

silent marsh
#

tbh i think lavalink v4 needs a lot more testing and the PR is far from ready

#

i wanted to leave a review on that PR last week but didn't get to it

wild marlin
#

We could just leave out lavalink for 0.16 for now if we want to get it out

karmic aurora
wild marlin
#

Also it needs the other or for everything to pass

karmic aurora
#

I merged the other CI fix

wild marlin
#

I am not going to be near a computer before tomorrow evening so I can have a look at it then

silent marsh
#

how is this even an issue

#

it makes for a good release timing excuse but man what is this

wild marlin
#

I guess the error makes sense if you have not understood how the crates system works with releases and such, but yeah a bit of a misguided issue

woven elbow
#

what can i do to help with getting entitlements merged 👀

wild marlin
opal juniper
#

I’m ok with someone else picking it up

wild marlin
#

@woven elbow, @jaunty jewel and @ocean nest if you have some time the comments should be resolved

ocean nest
#

which pr are we talking ab

#

if its the new dockerfile one theres just one small nit then lgtm

wild marlin
wild marlin
ocean nest
wild marlin
ocean nest
#

ill check on pc in a sec

ocean nest
#

even on web its not changed

Screenshot_2024-04-15_at_9.38.45_AM.png
wild marlin
#

Hmm it is for me, is there a refresh button on the top line?

#

I think the message might have gotten messed up for some unknown reason

silent marsh
#

I'm so afraid of making releases because I know I'll forget something

#

Happened again just now

wild marlin
autumn sequoia
#

(Please ping if you want my attention on something.)

wild marlin
#

@autumn sequoia Not really for the PR as I think it can be added later, but does the Digest trait not allow to do the streaming thing which we have talked abount before

autumn sequoia
#

The status of streaming signature verification currently appears to be "PR approved, but not yet merged"

wild marlin
#

Ahh makes sense, I thought you could use a hash digest for verification.

#

I gave you a small review most of it looks nice, just a few small things.

autumn sequoia
#

ferristhumbsup Lemme go through them right now

#

(And yeah, hash digests are in fact part of verification. It's just that there's a few other ✨Mathy Parts✨ that I wouldn't feel comfortable trying to do outside of the crypto crate maintained by actual cryptographers.)

wild marlin
#

Yeah makes sense, I think we can update it to use a possible streaming verifier at some point when it gets merged.

karmic aurora
wild marlin
#

Yeah

wild marlin
#

@silent marsh would we just reconnect if we got a rsv1

wild marlin
silent marsh
#

But the websocket connection is definitely unrecoverable from that

#

Yes, we would reconnect

#

Receiving the protocol error would mean disconnect(CloseInitiator::Transport) gets called

#

Which then sets the shard state to Disconnected with 0 reconnect attempts so far

wild marlin
#

Okay nice at least we don't die because I have heard of multiple incidents now.

wild marlin
silent marsh
#

I just realized that the lavalink code does not reuse TLS connectors whatsoever...

#

and then I tried adding that in but it's impossible to do it without exposing tokio-websockets as a dependency on NodeConfig

#

We should really get rid of lavalink in its current form

wild marlin
#

I think before a non-rc release we should get #2282, #2328 and #2330 merged. And then we should look into working on the lavalink update as well.

silent marsh
#

I'm finishing up the rustls 0.23 stuff for a stable release right now

#

kinda noticed that my filesystem got corrupted during that tho :D

wild marlin
#

Sounds fun

silent marsh
#

@wild marlin uhm is it intended that lavalink unconditionally disables tls

#

i see so many issues in this crate lol

#

why does the crate even have TLS features when ws:// and http:// is hardcoded

#

guess I'll fix that in my pr

#

the entire code is inherently flawed

wild marlin
#

I guess a proper rewrite should be on our list.

wild marlin
#

@autumn sequoia If you rebase it now all the errors should be gone.

autumn sequoia
wild marlin
wild marlin
#

@silent marsh Which features are we going to end up with with rustls?

silent marsh
#

looks something like this

#

i still need to figure out how to deal with people who don't enable a crypto backend

#

http will error lazily and gateway will error at compile time, which is pretty meh

wild marlin
opal juniper
wild marlin
karmic aurora
#

Probably captured from struct -> json -> compressed

wild marlin
#

Ah yeah that is possible as well

opal juniper
wild marlin
#

I am not quite sure what the best way is tbh

opal juniper
#

That works too, it’s mainly a difference between automated and serialization or building it manually (in a more consistent way)

#

The latter would be the easier to migrate to imo

wild marlin
#

Yeah that was what I was thinking

karmic aurora
#

I prefer the smaller dependency list of the builder

opal juniper
#

Is there a more efficient way to do this? My end goal is that I have a wrapper that takes any iterable and implements a string version to display all of it's elements via Display?

karmic aurora
# opal juniper

I also removed the clone bound by defining a Iterable trait, but the clone will just be a memcpy in your case since IntoIterator is implemented for &'a Vec<T> (and most other collections) so this isn't really an optimization.

use std::fmt;

trait Iterable {
    type Iter<'a>: Iterator<Item = Self::Item<'a>>
    where
        Self: 'a;
    type Item<'a>
    where
        Self: 'a;

    fn iter(&self) -> Self::Iter<'_>;
}

impl<T> Iterable for Vec<T> {
    type Iter<'a> = std::slice::Iter<'a, T>
    where
        Self: 'a;

    type Item<'a> = &'a T
    where
        Self: 'a;

    fn iter(&self) -> Self::Iter<'_> {
        self.into_iter()
    }
}

struct DisplayIterable<'a, T>(&'a T);

impl<'a, T, U> fmt::Display for DisplayIterable<'a, T>
where
    T: Iterable<Item<'a> = U>,
    U: fmt::Display,
{
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        for item in self.0.iter() {
            item.fmt(f)?;
            f.write_str(", ")?;
        }

        Ok(())
    }
}
karmic aurora
#

Here's the above with IntoIterator. I changed Clone for Copy to ensure it's only used on references, i.e. no deep clones

📎 file.rs
wild marlin
#

Your display implementation is a bit different than Siris's as it will write a extra , after the last element.

opal juniper
wild marlin
opal juniper
#

Yeah I ended up doing .map.collect.join(“,”)

wild marlin
#

👍

#

No, but this probably fits better in #1017541980273770528 if you have a specific use case.

karmic aurora
wild marlin
#

Lets continue query builder discussions in #1243086999817162804

wild marlin
silent marsh
#

I still believe that this shouldn't exist

#

If you look at the PR, the utility saves a whopping 7 lines of code in the example, half of which is due to reformatting

#

That doesn't sound worth the maintainance effort required to keep this up to date with new dependency releases

wild marlin
#

Well I disagree, I think this is specifically what the util crate is for

#

Like the snowflake module saves like a couple of lines of code.

silent marsh
#

In my view, what would go in the utility crate could be an integration with a web framework to provide a one-stop solution for the entire request parsing, body validation, etc. In the current state this is a renamed wrapper around dalek, which I just don't think is good enough of a case to add it.

wild marlin
#

So for it to be merged you want something like a tower layer added?

silent marsh
#

Something like that, or an integration with http

wild marlin
#

I think that is something that can be added, but I don't think it should be coupled with this pr

#

Then we have to change our development practices as we have generally been rather careful with not exposing crates.

silent marsh
#

Yeah but the entire thing that this PR does it re-expose dalek and call it a utility, which is a joke

#

It doesn't relieve any work from you like what snowflake utils do

#

You just replace dalek types with util types

wild marlin
#

okay, I guess I will add what you want in that case, because I would rather see this being added.

silent marsh
#

The only arguable case I've seen in favor of the PR in its current state was that it would make it a no brainer for users when deciding how to validate the requests

#

But having the example in its current state in my view is sufficient for that

silent marsh
wild marlin
#

So tower_http does have a way to set up validation of requests but I don't think there is any way to access the body from those which is rather annoying

#

Anyone have any good idea of how to handle this?

wild marlin
# silent marsh Also regarding this: the crate's utility as a utility is very limited if we don'...

This actually also goes against what @karmic aurora said which is probably the reason it has not been done: https://github.com/twilight-rs/twilight/issues/2169

GitHub
Signature validation utility · Issue #2169 · twilight-rs/twilight

What? Signature verification utility for webhook style bots. I was advised to design an API for adding this to twilight-util. There are a few approaches to this that come to mind: You can provide a...

eager birch
#

I would just like to put up that i think the 7 lines it saves are not representative of how much work and time it saves in a real application

karmic aurora
jaunty jewel
#

@karmic aurora https://github.com/twilight-rs/twilight/pull/2314#discussion_r1621800639
by this do you mean a trait returning certain structs (https://api.twilight.rs/twilight_mention/fmt/trait.Mention.html) or constructing format structs by themselves (https://api.twilight.rs/twilight_model/user/struct.DiscriminatorDisplay.html) ?

Mention in twilight_mention::fmt - Rust

Mention a resource, such as an emoji or user.

DiscriminatorDisplay in twilight_model::user - Rust

Display formatter for a user discriminator.

karmic aurora
#

I'm imagining just pub struct Bold<F>(pub F), but there might be a better, more convenient, API

lean geyser
#

ed25519 signature validation, the most controversial PR to cross Twilight's GitHub repositories in years lolshiba

jaunty jewel
opal juniper
karmic aurora
opal juniper
#

I tried the compressing the json into bytes but the test still fails

karmic aurora
#

at which step?

opal juniper
#

serde_json from_slice

karmic aurora
#

doesn't serde provide a helpful error message?

opal juniper
#

I think it just gives a format error

karmic aurora
#

you can inspect the json by first calling str::from_utf8

autumn sequoia
eager birch
#

I might be willing to write it, but I honestly think that writing it in a stable way is Not Worth It™️

#

afaik no rust web libraries are 1.0 yet

wild marlin
#

The issue with this that it is rather annoying to gain access to the body of a request because of how they have been made.

#

And if you make the body non generic it will break stuff

#

And then comes the other issues wuth service which means you have to implement your own futures and stuff as well, so you probably need pinning and everything gets annoying then.

eager birch
# wild marlin Hyper is

there isn't really anything to be made with hyper, though- iirc it doesn't have middleware, extractors... it just gives you a request, expecting a response

karmic aurora
#

tower::Service is relatively stable as it's a re-export of tower_service::Service (latest release was november 2019)

wild marlin
opal juniper
wild marlin
autumn sequoia
#

yeah I don't have the motivation to engage with this process anymore
https://github.com/twilight-rs/twilight/pull/2205#issuecomment-2143895751

I know you think it's not a lot of work, and it's not, but asking me to do this in more than one session is a great way to make me burn two full days on this instead of one.

GitHub
feat(signature-validation): add ed25519 signature validation to twi...

Implementing #2169. This PR is not complete, but I felt like making what I wrote just now public quickly so people know progress exists.

#

maybe if we were still in 2023 and I had the same spare time as I did when I decided "yeah sure I'll try getting a utility for this into the library I'm promoting"

#

my PR has the "Allow edits by maintainers" box checked. if you want to make a bunch of little changes to it, you can do it without funneling them through me

#

I promise I won't care

lean geyser
lean geyser
#

hat off just because I don't engage in the code these days

eager birch
#

i'm more then willing to take it over, but I'm not sure if that's something github supports

#

And yeah, the process does not seem to be particularly efficient

wild marlin
#

fully understandable, I am quite dissapointed with how the process have been.

eager birch
eager birch
#

should i make a pr for that

eager birch
#

or is adding copy something that can be taken care of in bugfixing

wild marlin
#

You are welcome to send in a pr for it

eager birch
eager birch
#

now that was a fast pull request

opal juniper
primal torrent
#

i'm looking at retries on 429 responses to twilight. would that be something that can be in default twilight or does that need a feature flag?

wild marlin
#

If it does not add any dependencies and is off by default it could be in without flags imo

primal torrent
#

not done it yet but in theory it shouldn't need any deps. if a cow works it shouldn't increase memory usage significantly either

karmic aurora
#

our request bodies are Body, so clones should already be ~free

primal torrent
#

yeah, currently doing that. just now trying to get all the types to behave cause we pass this stuff around deeply

#

without bypassing rate limits, not sure yet if that part works as expected yet

primal torrent
#

ok i was a bit to optimistic, gona need to do some more work to make it actually wait for the rate limited rather then just blindly retry

eager birch
#

Could a ResourceType be added for just the MEMBER_CURRENT, rather then all members?

#

It's currently not a problem for me because I don't use the guild members intent, but I can see it being one down the road.

silent marsh
#

I had (and still have) a patch for that and it was rejected about 2 years ago

#

I agree that it is extremely useful

eager birch
#

why was it rejected?

silent marsh
# eager birch why was it rejected?

Aa far as I remember, it was something along the lines of "discord doesn't separate this out as a new intent or model, so we don't want to treat it like one"

eager birch
#

I mean

#

It's kind of an intent

#

Same way as interactions are, really.

ocean nest
#

should i make a pr to change NonZero* to NonZero<T> that was added in the latest rust update or is that not worth bumping our msrv

wild marlin
#

I don't think it really adds anything to our usecase

#

Since we always use the same T

wild marlin
#

Would appriciate a quick review, it does not have many changes.

karmic aurora
wild marlin
#

The issue is more that we do some funky stuff with nightly, that being we build the std and such.

#

Otherwise you could make a better solution.

wild marlin
#

Thanks

wild marlin
#

@karmic aurora I have turned on auto-removal of branches on the repo now (also did it on the gateway-queue repo)

wild marlin
#

@opal juniper Do you not still have a incorrect test?

opal juniper
#

Yeah never got around to getting the byte representation for the test

wild marlin
opal juniper
#

Oh yeah that’s trivial, I’ll update that when I get home

ocean nest
wild marlin
#

@opal juniper This is the old json for the compression test, can you send back the changes then I figure out the byte array for you:

{"code": "twilight-rs", "guild": {"id": "745809834183753828", "name": "Twilight", "splash": "9d70baddb90a36979015ace8aadeb92e", "banner": null, "description": null, "icon": "089f35e5d0a28ff4927a7f825d71f41c", "features": ["THREE_DAY_THREAD_ARCHIVE", "WELCOME_SCREEN_ENABLED", "NEW_THREAD_PERMISSIONS", "THREADS_ENABLED", "MEMBER_VERIFICATION_GATE_ENABLED", "VERIFIED", "VANITY_URL", "NEWS", "INVITE_SPLASH", "ANIMATED_ICON", "PREVIEW_ENABLED", "COMMUNITY"], "verification_level": 3, "vanity_url_code": "twilight-rs", "welcome_screen": {"description": "Support and development of Twilight, the ecosystem of Rust libraries for the Discord API. https://twilight.rs", "welcome_channels": [{"channel_id": "745811192102125578", "description": "Ask for help with Twilight", "emoji_id": "832667826547851305", "emoji_name": "rust"}, {"channel_id": "745811216579952680", "description": "Talk about Twilight development", "emoji_id": "745916458541776926", "emoji_name": "twilight"}, {"channel_id": "745813247705481246", "description": "Read library update changelogs", "emoji_id": null, "emoji_name": "\ud83d\udce3"}, {"channel_id": "745811486819221544", "description": "Keep up with GitHub development", "emoji_id": null, "emoji_name": "\ud83d\udcd6"}]}, "nsfw": false, "nsfw_level": 0}, "channel": {"id": "745809834183753832", "name": "info", "type": 0}}
opal juniper
#

Ok we can do that

karmic aurora
ocean nest
#

aliasing it makes sense for dry code i guess we’ll see what happens

wild marlin
wild marlin
primal torrent
#

done

wild marlin
#

@silent marsh that sounds complicated, and I am not sure I can see it for me at the moment but I will look forward to a poc (re: <#1254760954194038794 message>)

silent marsh
#

It is definitely complicated but I can see that being useful

#

I've seen so many questions in #1017541980273770528 regarding comparing state before and after cache updates

#

ugh no, there goes my idea XD

#

This won't work because of the Event enum. It would work for individual structs

#

Actually nvm, that's okay

#

We can make Event return () and the specific updates return a type

silent marsh
#

I only implemented it for the channel events for now, it gets repetitive

#

This lets you do cool things like

let channel_before_update = match cache.update(&channel_update) {
    UpdateResult::Processed(old_channel) => channel,
    UpdateResult::Ignored => panic!("I never disabled channel processing in the cache"),
};```
(I'd add an unwrap to `UpdateResult` if others like this POC)
silent marsh
#

@wild marlin I have a suspicion that the unavailable guilds handling is incorrect

#

Based on what a user is telling me it seems that the cache state is just plain wrong

wild marlin
silent marsh
#

Yeah

#

I'm not 100% sure to be honest

#

But there's a lot of issues with the cache changes recently

#

They're basically all missing getters

silent marsh
#

All I did was update twilight in the gateway proxy and apparently now the guild unavailable state is just wrong

#

But the user I'm in contact with regarding this hasn't provided me any logs or payloads, so I can't really tell what is going on

wild marlin
#

So before it would not work at all if the guild became unavailable since it would just fail to deserialize

#

So if it was right before I am not sure it was right before

#

It might be wrong as well now though

silent marsh
#

I'm not 100% sure who is to blame here

#

Could very well be a bug in discord.py (the library they are using)

#

Without payloads we won't know

wild marlin
#

Have you tried with your own bot? If you have identifies to spare.

silent marsh
#

The only change I did was sending the unavailable guilds via a GUILD_CREATE instead of a GUILD_DELETE btw

wild marlin
#

Else I can try it out but I don't think my bot is in enough guilds for one of them to be unavailable

silent marsh
#

since now twilight-model allows for that and discord docs document that as valid

wild marlin
#

Hmm, maybe the serialize is wrong?

silent marsh
#

I got this from them

#

Apparently the guild isn't marked as unavailable properly in their bot yet all the fields are unpopulated (i.e. it is supposed to be unavailable)

#

discord.py just completely ignores GUILD_CREATE payloads with unavailable set to true, which is pretty fair I think

wild marlin
#

Yeah I guess so

#

Then the question is if it is something we should look into solving or we should let the bot dev solve?

silent marsh
#

I don't really know yet

wild marlin
#

Hmm I think it is in your end @silent marsh

#

GuildCreate should only be used if it is unavailible from the start I think

#

Otherwise GuildDelete should be used

silent marsh
#

Are you sure?

#

Meh, I'm not 100% sure

#

To me this means that guild creates with unavailable guilds are allowed in GUILD_CREATE events for backfilling OR when the bot joins a new unavailable guild

wild marlin
#

I think that is the key point there

#

When it is not part of initial connection it is not allowed

silent marsh
#

w/e, I switched back to sending guild delete events

wild marlin
silent marsh
#

Ah that would explain the issues I've been running into

#

Uhh

wild marlin
#

Though on the other hand it means that we are probably failing to deserialize some guilds for other reasons.

silent marsh
#

I think you should add a test case for an available guild

#

In the deserializer for guild_create

#

currently it only tests an unavailable guild

#

and from my understanding the issue was never the deserialization of unavailable guilds, but rather available guilds being deserialized as unavailable?

wild marlin
#

Yeah because the normal guild deserilizer fails and then it fits into unavailible guild.

#

So it kinda shadows the error.

silent marsh
#

Meh, this is hard to test

#

Because it points to our guild struct being wrong or failing to deserialize

#

We definitely need to fix both

wild marlin
#

Yeah which is why I would be really interested in test data

silent marsh
#

But a test case could at least ensure that we see an error with a guild payload we can't deserialize

wild marlin
#

Seems that their issue was somewhere else, but I think it is still a good idea to ensure that it does not do odd stuff.

#

I will fix up the pr tomorrow probably.

primal torrent
#

did some reviews but gave up on the retries on rate limit stuff for now. i don't understand the internals of the twilight request sending code with the rate limiter enough to get it to behave

#

it's all too connected in ways that are very confusing and hard to follow

primal torrent
#

ok so it's easier for me to basically just wrap the entire client, duplicate the methods i need and handle 429 retries that way. so guess i'm going that route for now. wanted to upstream it but it just doesn't seem doable

opal juniper
wild marlin
opal juniper
primal torrent
opal juniper
#

Fixed

silent marsh
#

@wild marlin I think it's time we decide how to properly approach the rustls upgrade

#

The issue is that no default features won't compile because a crypto backend needs to be selected

#

And even if it would compile - it would result in runtime panics from rustls

#

I don't think that choosing a preferred implementation ourselves is good. aws-lc-rs has legitimate usecases (supports more and safer key exchange algorithms and is generally faster than ring)

#

So I think that the only reasonable way forward is to not panic in order to unbreak powerset CI and instead emit a compile time warning

#

IMO this is the best way because it would technically allow the user to provide their own crypto implementation if they call the rustls method to make it the global default

wild marlin
#

sorry, just despawned for a week, will have a look at it.

silent marsh
#

and what do we do about MSRV? I wouldn't mind moving along tokio to 1.70, we're on a rather old MSRV by ecosystem standards already

ornate onyx
#

Hy

wild marlin
wild marlin
wild marlin
silent marsh
silent marsh
wild marlin
#

Hmm, yeah it says so, I just remember running into issues, but maybe I remember wrong.

wild marlin
karmic aurora
silent marsh
silent marsh
#

I'll do a last release of tokio-websockets 0.8 after bytes 1.7.0 is released (should happen today or tomorrow), and then a new 0.9 with MSRV 1.79 for the split_at_mut_unchecked and async traits. We should bump MSRV in twilight accordingly, update rustls, fix the guild create deserialization and then release a stable 0.16.

silent marsh
silent marsh
silent marsh
#

aaaand we're gatekept by hyper-rustls' shortsightedness :D
<#1015156984007381033 message>
Let's see what they respond

wild marlin
#

Time for with_provider_and_*_arc

silent marsh
#

ugh

#

rustls is such a cool project but they really gotta get down to reality when it comes to crate maintainer pov for downstream

#

I hope Dirkjan sees it and decides on a way forward now that I proposed two ways

primal torrent
#

What are they doing?

silent marsh
primal torrent
#

Ah ,dependency loop issues?

silent marsh
#

they have a solution for that: users can configure a default provider in rustls via feature flags OR install a global default via a method, which rustls can then always fetch on the fly when you call its new methods

#

but of course they feature gate the methods for that behind the provider flags in their own crates XD

lean geyser
#

well yes, why would you want ring and aws-lc-rs in your dependency tree at once if you only use one?

silent marsh
#

no, you did not get what i meant

#

the methods that it calls do not actually require having those installed/added as dependencies!

#

since rustls takes care of everything related to the selection on its own if you let it

#

See https://docs.rs/rustls/latest/rustls/client/struct.ClientConfig.html#method.builder :

Create a builder for a client configuration with the process-default CryptoProvider and safe protocol version defaults.

vs https://docs.rs/rustls/latest/rustls/client/struct.ClientConfig.html#method.builder_with_provider :

Create a builder for a client configuration with a specific CryptoProvider.

Now read https://docs.rs/rustls/latest/rustls/crypto/struct.CryptoProvider.html#using-the-per-process-default-cryptoprovider and start questioning why hyper-rustls does not use the process-default method UNLESS you enable a specific crypto provider

ClientConfig in rustls::client - Rust

Common configuration for (typically) all connections made by a program.

CryptoProvider in rustls::crypto - Rust

Controls core cryptography used by rustls.

wild marlin
#

Tbh I am not too sure if we should support simd-json, it's mostly a pain

#

Especially Seeing #1271866310468304966

#

@valid oracle apparently simd-json breaks things.

valid oracle
#

I removed it from serenity ages ago

swift gull
#

Are there any realistic benchmarks or people relying on simd-json in production who have seen a meaningful performance improvement?
I'm just guessing here, but I can't imagine a scenario where JSON serialization would be the bottleneck of the bot.

valid oracle
#

simd-json is not a performance improvement for serenity,
is somewhat of a Cunningham's Law trigger, but we have seen no numbers that say simd-json are worth the maintenance burden and possible footgun

#

I removed simd-json after discord had a slight whoopsie and was sending fucked avatar hashes for some users, and my first thought was "disable simd-json, maybe it's fucking with things"

silent marsh
#

upstream development is still active so we should just report these bugs instead of keeping our grudges to ourselves, otherwise nothing will happen

#

I'd be really pissed as a library maintainer if people kept complaining about my library being buggy but never filed bug reports

silent marsh
#

@wild marlin is that deserialization issue in simd-json the reason why our guild create deserialization fails?

#

or is there more to it

wild marlin
silent marsh
#

Ah nice, very good

wild marlin
#

@silent marsh re your tws pr, would the idea then be to install the provider further up eg in twilight or in the users app?

silent marsh
#

the benefit for twilight is that Connector::new is unconditionally available

#

so we can actually use it in the gateway even if no crypto provider is enabled via crate features

silent marsh
#

could we please get #2366 fixed up and merged? I want to do an MSRV bump after that

knotty ferryBOT
#

chore(clippy): Resolve Clippy 1.80 lints, authored by @Erk- <t:1723320917:R>

wild marlin
silent marsh
#

awesome, thanks

#

ill wait for CI and then approve and merge

#

what do we want to bump MSRV to? follow tws and go 1.79? or do we want anything more recent?

#

i think 1.79 gives us enough headroom for now

wild marlin
#

tbh I would be fine with either that or simply just taking the latest released version when we release.

silent marsh
#

let's bump to 1.79 for now then and reconsider this at release

#

btw i got too tired of hyper-rustls api shenanigans so for the rustls 0.23 PR i decided to build ClientConfigs ourselves... it's basically the same amount of verbosity as using the hyper-rustls builders

#

lol, failing MSRV check means clippy doesn't run... Pain :D

#

do we wanna bump MSRV in a separate PR and ignore CI or can you bump it in the clippy fix PR?

wild marlin
silent marsh
#

the READMEs and stuff will need to be updated as well

wild marlin
#

two secs

wild marlin
#

Remember to write both things in the merge commit message

silent marsh
#

sorry was a bit busy, will look again now

#

oh fun, clippy CI is failing

#

i guess higher msrv allows for some new methods in lint resolutions

#

yup those are 1.77+

silent marsh
#

hope you don't mind me force pushing your branch with the clippy suggested fixes

silent marsh
#

anyone opposed to depending on the rc version of simd-json at least for now?

#

I think the deserialization fix for us is critical enough - in the current state, simd-json support is broken and I'd prefer it to work with an rc dependency rather than keeping it broken for now

#

we can always upgrade to a stable release later without making breaking changes on our end

wild marlin
#

I think it is OK to do so

silent marsh
silent marsh
#

@wild marlin any particular reason for the MustBeBool thing?

#

having MustBeBool in our public API feels... wrong

wild marlin
silent marsh
#

hmm right

#

i wonder if we can somehow not have this as a struct field

#

something like a private type with it that we use for deserializing

#

and then our deserializer converts it into the other type if it deserialized into the intermediate unavailable guild type

wild marlin
#

Can't remember if I tried that I think I tried the one you linked until I saw it used serde_json. But it is also a few months since I looked at it.

silent marsh
#

this works for me

#

(mod mustbe is copied from your PR)

#

I would prefer this so that we can hide the MustBeBool type

wild marlin
#

That looks fine, then we can just use upstream mustbe as well

#

Feel free to push a commit with that if you want

silent marsh
#

Sure, will do

#

Not sure about the naming but it doesn't really matter if it's private xD

wild marlin
#

(though I am not a fan of monostate using macros so baybe we can just keep mustbe)

silent marsh
#

sure

#

pushed

#

oh i missed the cache part, woops

wild marlin
silent marsh
#

with that PR and the rustls and simd-json ones I'd be down for a new rc release

#

for a stable release i want the lavalink v4 stuff in

wild marlin
silent marsh
#

I can take a look at the code, but I'm not familiar with the APIs and don't use them myself

#

@wild marlin are you okay with me nuking all unused trait impls and derives in mustbe.rs?

#

we don't need eq and hash

#

and actually we don't need serialize either