aatxe
commented
6 years ago Thanks for opening an issue, and sorry you've run into trouble!
First, I just wanted to point you to the IrcReactor API, in case you hadn't seen that. It wraps up dealing with IrcClient::new_future and PackedIrcClient (whose documentation currently says approximately "don't use me unless you know the library impl") and joins on the futures for you. I just tried sending a KILL to the example at examples/reactor.rs and the program exited appropriately (although I'm actually not 100% certain why).
Explanation
Assuming you wish to proceed with doing the tokio parts yourself, let's talk about what's happening! The future that you get from the PackedIrcClient needs to be driven in order to actually send messages to the server you're connected to, and it's implemented as the send-half of the connection sending everything it receives from a channel. If at any point you attempted to send a message to the client, you should wake up this sending-half which will then notice the connection is closed. Without that, you are right to notice that it's in a deadlock situation, but it's actually only caused by clean connection drops. If a connection drops messily (e.g. computer goes to sleep, wifi drops out, etc.), the future driving the read half will eventually try to send a ping to make sure the connection is still alive causing the chain reaction that gets you out. However, if the connection drops cleanly and nothing ever tries to send afterward, the send half will just happily chug along never knowing what has happened (because "chug along" actually means wait eternally to be awoken for a message that shall never come).
Plausible Fixes
So, this explanation hopefully makes it clear what the bug is: we need to get that future to end when the connection closes cleanly. I can imagine a few ways to do this. The absolute simplest way involves adding a NoOp command to irc::proto::Command that one would send after the connection is closed cleanly (e.g. after you get KILLed or you send a QUIT). This would work fine for the IrcReactor scenario because it'd be transparent to end users, but in your situation, you'd have to add something like .map(|_| client.send(NoOp)) which is not great.
However, I believe the better alternative is to replicate more of the ping logic in the sending half of the IrcTransport. Currently, sends will check if a ping has already timed out, but it doesn't interact with the mechanisms that cause the receiving half to wake up every so often to send its own pings and so forth. If we replicate that on the send half, everything should work out.
Perhaps I've said too much, but this is a bug and that last bit describes generally how to fix it. If you'd like to take a crack at it, that'd be great (and I'd be happy to answer any questions)! If not, I'll try to get a fix in ASAP. Thanks again for the report!
I spent a bit more time than I'd like to admit on getting the library to work nicely w.r.t. connection drops. It turns out that example code here actually results in a deadlock. The deadlock can be triggered by setting the code up to run as intended, and then klining the connection. I've written a bit of example code here.
To fix it, the
futurefrom thePackedIrcClienthas to be spawned separately from the client stream future (or at least this is how I fixed it). Maybe there's a better way to do this.I'm still trying to wrap my head around how to best deal with this, but once I'm familiar with the library, I might actually open up a PR with a docs fix for this.