Panic caused by an expect call

[Kerollmops]

Hey,

Thank you for the great work, this library works fluently but I encountered a little runtime panic, on this line, I am not able to reproduce that as I was just connected to Twitch, so... Something was received and that library didn't like it.

twitchchat/src/runner/async_runner.rs

Line 308 in 6d41348

.expect("msg identity conversion should be upheld")

I also saw that you were working on a big 0.15.0 refactor so, I guess this bug report is irrelevant.

Thank you for your help :)

[museun]

Yes, that expect has caused numerous problems. It assumes Twitch won't change their messages + they adhere to the IRC spec, which sadly neither are true.

I wanted to make it fallible but that'd break the 0.14.x semver and I didn't want to bump to 0.15 just for changing a fn() -> Message to a fn() -> Result<Message> which would break everything that uses this. BUt I'll make sure the things that could, in theory, be fallible are marked as fallible.

That expect is actually about an internal conversion. I convert from a specific message to an enum of all messages and then back to the specific message (a->b->a which shouldn't fail, but it does).

I'm also going to redo the parser to be way less strict/do less "error detection". It'll be more lenient to how Twitch structures their messages.

[museun]

I should add: the 0.15.0 refactor is rather large, but it basically decomposes all of the functionality into individual components that one can put together however they see. This design also allows for better abstractions over Read/Write,AsyncRead/AsyncWrite,Stream/Sink (and Iterator / Stream ) all with similar functionality.

This design also lets me move all of the I/O to external crates which gets closer to my goal of sans-io with the crate. The core crate should be usable with just the standard library, and for async stuff with just minimal deps.

I'm waiting for a few key deps to be stabilized/updated (mostly around the websocket/tokio world), thus stalled for now.

[Kerollmops]

Yeah, I read the PR description and it seems to be a lot of work, I should thank you for all your work again!

The sans-io feature is impressive indeed and it seems to be the way all async crates should go and not focus only on tokio and/or async-std :)

And no problem, I will wait!

[norcalli]

Considering that this .expect basically makes the library useless, don't you think that's worth pushing a quick 0.15? It's not like pushing 0.16 for the big refactor would be that much different...

I was about to fork the library when I found this issue saying that it's a known problem.

[norcalli]

Should also probably yank 0.14. Having a panic in the middle of parsing code means that it is functionally a broken release.

[museun]

Do you happen to have a test case @norcalli (just the bad input string)? so I can write a unit test for the fixed release

But yes, I'm been super busy these last few weeks because of real life obligations. so I've been pushing it off.

(The problem is likely twitch changes message formats without making any announcements. Their types aren't really documented, and the ones that are are usually incorrect.)

If I can see an example of a bad message, I can see how I can massage the irc message parser to be more resilient to future-unknown message format changes from Twitch

But I agree, an 0.15 release could just change the parsing code to fail early. That specific expect should never happen because its just an internal roundtrip. (its needed to rate limit the 'whole system'. I'm planning on removing rate limiting and providing a more atomic/composable approach that users can apply their own rate limiting to, instead of a global one thats intertwined with the state machine).

Also, with some recent changes in the smol ecosystem, I can rely on a local executor to spawn tasks which'll greatly reduce the complexity of running multiple internal loops at the same time. A local executor would still play fairly with the big async runtimes, so this should be fine. (This current design does everything in the same loop/future because I didn't want to rely on any runtime or extra threads)

[museun]

Some things I will want to change:

• Make the Encodable trait use dynamic dispatch so it can be boxed, currently its not object safe because its generic over a writer.
• Rework the heartbeart/timeout/rate limiter to be more composable (and optional)
• Remove that expect and that dumb re-allocation.
• Perhaps move away from the MaybeBorrow type and just allocate the strings -- the indexing-as-a-way-to-do-self-ref-structs is a bit tedious and annoying.
  • if I do just use a struct of strings, then I can use TryFrom/TryInto which'll also let me get read of FromIrcMessage/IntoIrcMessage (the lack of a GAT in the std Borrow/TryFrom/TryInto to be generic over lifetimes is why I can't use it currently. I need to reborrow types Cow<'a, str> -> Cow<'a, str> + 'a basically. but thats not possible with the standard traits.
• Restructure the crate so external deps aren't so intertwined
• Support Sink/Stream in more places

I think I can provide tokio02 and tokio03 support (I'll have to check the various TLS libraries this crate supports.)

I, honestly, don't even want to provide runtime support at all -- all the current runtime code does is connect and returns a (AsyncRead, AsyncWrite). Ideally this should be up to the user, do your connect logic, call my register function, wait_for_ready and then, pass it to the reader/writer (decoder/encoder) and do the rest on your end.

I should turn this into a tracking issue.

Another possible change: switch from log to tracing. and do more logging

[norcalli]

I ended up writing my own library to do it all since it was faster that way and I could make the parsing more flexible. I do agree, all the runtime code is probably a waste of time. The Rust async ecosystem is still tremendously immature and trying to support it all will stretch you thin. You should just support the stuff you actually use if you still use this lib, or make it agnostic.

[museun]

Yeah, I'm actually debating on archiving this project or giving it to someone else. The last month, in RL, has been rather busy and hectic and I can barely find time to do any personal programming.

I apologize to everyone for being so delayed/distance, I'll write up a summary and make an issue if anyone wants to have stewardship (and I'll also lay out a little plan of what to fix/remove. (The fix is rather simple, just return a Result and let the user either discard the message or panic themselves))