search

aatxe / irc

the irc crate – usable, async IRC for Rust
Mozilla Public License 2.0
530 stars 97 forks source link

IRC Server panics during connecting to server #109

Closed DoumanAsh closed 6 years ago

DoumanAsh commented 6 years ago 
Jan 27 16:47:09.142 WARN Bot disconnected. Error: The connection timed out due to no ping response., location: bot/src/main.rs:79
thread 'main' panicked at 'Box<Any>', /home/pi/.cargo/registry/src/github.com-1ecc6299db9ec823/irc-0.12.8/src/client/server/mod.rs:822:36
note: Run with `RUST_BACKTRACE=1` for a backtrace.

My bot got disconnect(first trace) https://github.com/DoumanAsh/roseline.rs/blob/master/bot/src/main.rs#L79

Then goes for re-starting IRC, but it seems not every error are gracefully handled by irc. I doubt it will be possible to reproduce at all for me(it seems to be due to some troubles with my router)

Do you think there should be a way gracefully to handle this panic inside irc? https://github.com/aatxe/irc/blob/stable/src/client/server/mod.rs#L822

aatxe commented 6 years ago

Ah, thanks for point this out! The direct answer seems to be to somehow pass the outgoing_future out to the user and have them run it themselves, but that's unsatisfying to me. I'm unsure of any other direct solutions because spawn requires the error type to be (). I would accept a patch that fixes this though.

On the other hand, I've been working on an API (irc-reactor branch) that will hopefully enable a path where there are no panics or unwraps, rather than just fewer. It currently hits this code, but since it's internal, I don't mind getting the outgoing_future out and then it should be straightforward to avoid this. This would eventually result in the removal of new_future altogether, in favor of using this new API. Does this API allow you to do everything you need for roseline?

DoumanAsh commented 6 years ago

Hm... Well if it will be possible for my handlers to return future it would suffice.

Maybe it would be better to allow user to provide way for handlers that return futures?

P.s. to be honest I'm thinking of moving away from plain tokio to Actor based model provided by nice actix library which makes code much more easily to reason about. But in this case I'll go for proto instead of client part of irc :)

aatxe commented 6 years ago

Good point! I changed register_server_with_handler and register_future to both be more generic (matching the typical Futures API more). Thanks! I'll update this issue when irc-reactor actually eliminates the panic you're experiencing.

DoumanAsh commented 6 years ago

That's good. Thanks!

aatxe commented 6 years ago

Fixed in 67cca339a76b2cc9ad0027cd3636d97c4729ec18 (though it changes the types for IrcServerFuture a bit).