Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't rely on RPL_YOURHOST parsing for connectivity checking #239

Closed
osa1 opened this issue Aug 18, 2020 · 6 comments · Fixed by #244
Closed

Don't rely on RPL_YOURHOST parsing for connectivity checking #239

osa1 opened this issue Aug 18, 2020 · 6 comments · Fixed by #244

Comments

@osa1
Copy link
Owner

osa1 commented Aug 18, 2020

I discovered this while reviewing #237 (see my comment). We currently use the server name when sending PINGs to a server to check connectivity, but we can't do that if we don't know the server name, which we currently parse from the RPL_YOURHOST message. When parsing fails servername stays None, and send_ping just returns:

pub(crate) fn send_ping(&self, snd_irc_msg: &mut Sender<String>) {
self.inner.borrow_mut().send_ping(snd_irc_msg)
}

This breaks connectivity checks when we can't parse RPL_YOURHOST for some reason. One example of this happening is when connecting to irc.gitter.im. Gitter's IRC server is not following the standards too closely and caused problems before (#214, #211). Currently RPL_YOURHOST from Gitter looks like this:

Msg { pfx: Some(Server("irc.gitter.im")), cmd: Reply { num: 2, params: ["osa1", " 1.10.0"] } }

This is quite different than how RFC 2812 defines it:

       002    RPL_YOURHOST
              "Your host is <servername>, running version <ver>"

What to do when we can't parse RPL_YOURHOST? I see a few options:

  • Send an empty PING message. The error response will reset the connectivity timer. The problem with this is that we'll see error responses in the server tab. Example: helix.oftc.net: osa1 PING Not enough parameters
  • Send a PING to yourself. This won't work if we're stuck in nick selection (not sure if this can happen). In that case we can fall back to (1). I just tried this on OFTC and it seems to work.
  • Send a HELP message to the server, which doesn't require arguments so will always work. (Note: HELP is ignored by irc.gitter.im)
  • (Other ideas?)

Note that (1) and (2) works on irc.gitter.im, which doesn't seem care about the PING argument at all and always responds with a PONG immediately. (3) doesn't work as it doesn't seem to support HELP message.

cc @trevarj

@osa1 osa1 added this to the 0.6.1 milestone Aug 18, 2020
@trevarj
Copy link
Contributor

trevarj commented Sep 4, 2020

@osa1 I found this on https://www.irc.com/dev/docs/refs/numerics/002.html

The <servername> may also include other information depending on the server software, for example irc.example.com[203.0.113.15/6667], so we instead recommend relying on the prefix or the RPL_MYINFO (004) numeric if knowing the current server hostname is desired.

Maybe to just rewrite parse_servername to use the prefix?

@osa1
Copy link
Owner Author

osa1 commented Sep 4, 2020

Good idea @trevarj . Would be good to compare values parsed currently vs. the prefix, and whether using prefix would work for pinging. I just tried irc.gitter.im and PING irc.gitter.im worked fine.

@osa1
Copy link
Owner Author

osa1 commented Sep 4, 2020

Another idea is to use the prefix as a fallback, when current parser fails.

@trevarj
Copy link
Contributor

trevarj commented Sep 4, 2020

This appears to work for all my servers, including gitter

                // First try to use prefix, which should have the server name
                if let Some(Pfx::Server(server_name)) = pfx {
                    self.servername = Some(server_name.to_owned())
                } else {
                    // Try to parse message for server name
                    match parse_servername(params) {
                        None => {
                            error!("Could not parse server name in 002 RPL_YOURHOST message.");
                        }
                        Some(servername) => {
                            self.servername = Some(servername);
                        }
                    }
                }

@osa1
Copy link
Owner Author

osa1 commented Sep 4, 2020

@trevarj sounds good. Some suggestions:

  • Let's do all of this parsing in parse_servername (it should take a full message as argument) and test it toroughly using responses of some of the widely used servers. I can add some tests based on servers I'm connecting to if you're going to submit a PR.

  • Let's use "Your host is ..." parsing as we do today first, and use prefix if that fails. Reason: it's battle-tested and we know it works well on standard-conforming servers.

@osa1 osa1 closed this as completed in #244 Sep 4, 2020
osa1 added a commit that referenced this issue Sep 4, 2020
Closes #239

Co-authored-by: Ömer Sinan Ağacan <omeragacan@gmail.com>
@osa1
Copy link
Owner Author

osa1 commented Sep 4, 2020

If anyone's following, the reason why this issue doesn't cause disconnects in irc.gitter.im is because the server sends a PING every 30 seconds:

[2020-09-04T15:22:33Z] DEBUG [libtiny_client/src/lib.rs:452] parsed msg: Msg { pfx: None, cmd: PING { server: "irc.gitter.im" } }
[2020-09-04T15:23:03Z] DEBUG [libtiny_client/src/lib.rs:452] parsed msg: Msg { pfx: None, cmd: PING { server: "irc.gitter.im" } }
[2020-09-04T15:23:33Z] DEBUG [libtiny_client/src/lib.rs:452] parsed msg: Msg { pfx: None, cmd: PING { server: "irc.gitter.im" } }
...

So even if we never send a PING to irc.gitter.im we can maintain a connection.

osa1 added a commit that referenced this issue Sep 4, 2020
@osa1 osa1 modified the milestones: 0.6.1, 0.7.0 Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants