telepathy-idle: Connection: disconnect if we get no PONG for our PING
Will Thompson
wjt at kemper.freedesktop.org
Fri Nov 2 04:56:49 PDT 2012
Module: telepathy-idle
Branch: master
Commit: eb437c2f0bbc7061570889a825b131969982cc9a
URL: http://cgit.freedesktop.org/telepathy/telepathy-idle/commit/?id=eb437c2f0bbc7061570889a825b131969982cc9a
Author: Will Thompson <will.thompson at collabora.co.uk>
Date: Tue Oct 30 15:52:09 2012 +0000
Connection: disconnect if we get no PONG for our PING
Previously we'd just send out a PING into the æther every n seconds, and
pay no attention to whether we ever got an answer. So it was perfectly
possible for the connection to just sit there until the TCP timeout
kicks in, which I think is a matter of hours by default.
With this patch, if we haven't heard a PONG 3 keepalive-intervals after
sending a PING, Idle throws its toys out of the pram.
This has been tested as follows:
* Put my laptop on a wired and wireless network simultaneously.
* Connect to an IRC server. (The wired network is used.)
* Pull the network cable out. Idle is too stupid to realise the link it
was using is gone, and because we're still ostensibly online, nothing
tells it to disconnect.
* Wait keepalive-interval * 4, and watch the connection get
disconnected.
It works both with a direct connection to Freenode, and with a
connection over an SSH tunnel to irssi-proxy.
---
src/idle-connection.c | 60 ++++++++++++++++++++++++++++++++++++++--
src/idle-parser.c | 1 +
src/idle-parser.h | 1 +
tests/twisted/connect/ping.py | 19 +++++++++++--
4 files changed, 75 insertions(+), 6 deletions(-)
diff --git a/src/idle-connection.c b/src/idle-connection.c
index 3fd8905..f4cf816 100644
--- a/src/idle-connection.c
+++ b/src/idle-connection.c
@@ -52,6 +52,7 @@
#include "extensions/extensions.h" /* Renaming */
#define DEFAULT_KEEPALIVE_INTERVAL 30 /* sec */
+#define MISSED_KEEPALIVES_BEFORE_DISCONNECTING 3
/* From RFC 2813 :
* This in essence means that the client may send one (1) message every
@@ -146,6 +147,11 @@ struct _IdleConnectionPrivate {
IdleServerConnection *conn;
guint sconn_status;
+ /* When we sent a PING to the server which it hasn't PONGed for yet, or 0 if
+ * there isn't a PING outstanding.
+ */
+ gint64 ping_time;
+
/* IRC connection properties */
char *nickname;
char *server;
@@ -207,6 +213,7 @@ static IdleParserHandlerResult _erroneous_nickname_handler(IdleParser *parser, I
static IdleParserHandlerResult _nick_handler(IdleParser *parser, IdleParserMessageCode code, GValueArray *args, gpointer user_data);
static IdleParserHandlerResult _nickname_in_use_handler(IdleParser *parser, IdleParserMessageCode code, GValueArray *args, gpointer user_data);
static IdleParserHandlerResult _ping_handler(IdleParser *parser, IdleParserMessageCode code, GValueArray *args, gpointer user_data);
+static IdleParserHandlerResult _pong_handler(IdleParser *parser, IdleParserMessageCode code, GValueArray *args, gpointer user_data);
static IdleParserHandlerResult _version_privmsg_handler(IdleParser *parser, IdleParserMessageCode code, GValueArray *args, gpointer user_data);
static IdleParserHandlerResult _welcome_handler(IdleParser *parser, IdleParserMessageCode code, GValueArray *args, gpointer user_data);
static IdleParserHandlerResult _whois_user_handler(IdleParser *parser, IdleParserMessageCode code, GValueArray *args, gpointer user_data);
@@ -703,6 +710,7 @@ static void _connection_connect_ready(GObject *source_object, GAsyncResult *res,
idle_parser_add_handler(conn->parser, IDLE_PARSER_NUMERIC_WHOISUSER, _whois_user_handler, conn);
idle_parser_add_handler(conn->parser, IDLE_PARSER_CMD_PING, _ping_handler, conn);
+ idle_parser_add_handler(conn->parser, IDLE_PARSER_PREFIXCMD_PONG, _pong_handler, conn);
idle_parser_add_handler_with_priority(conn->parser, IDLE_PARSER_PREFIXCMD_NICK, _nick_handler, conn, IDLE_PARSER_HANDLER_PRIORITY_FIRST);
idle_parser_add_handler(conn->parser, IDLE_PARSER_PREFIXCMD_PRIVMSG_USER, _version_privmsg_handler, conn);
@@ -808,15 +816,49 @@ static gboolean keepalive_timeout_cb(gpointer user_data) {
IdleConnection *conn = IDLE_CONNECTION(user_data);
IdleConnectionPrivate *priv = conn->priv;
gchar cmd[IRC_MSG_MAXLEN + 1];
- gint64 ping_time;
+ gint64 now;
if (priv->sconn_status != SERVER_CONNECTION_STATE_CONNECTED) {
priv->keepalive_timeout = 0;
return FALSE;
}
- ping_time = g_get_real_time();
- g_snprintf(cmd, IRC_MSG_MAXLEN + 1, "PING %" G_GINT64_FORMAT, ping_time);
+ now = g_get_real_time();
+
+ if (priv->ping_time != 0) {
+ gint64 seconds_since_ping = (now - priv->ping_time) / G_USEC_PER_SEC;
+
+ if (seconds_since_ping > priv->keepalive_interval * MISSED_KEEPALIVES_BEFORE_DISCONNECTING) {
+ GCancellable *kill_me_now = g_cancellable_new();
+
+ IDLE_DEBUG("haven't heard from the server in %" G_GINT64_FORMAT " seconds "
+ "(more than %u keepalive intervals)",
+ seconds_since_ping, MISSED_KEEPALIVES_BEFORE_DISCONNECTING);
+
+ /* Passing a cancelled cancellable to g_io_stream_close_async() stops us
+ * waiting for a TCP-level reply from the server which we already know
+ * has gone away. Quoth the docs for g_io_stream_close():
+ *
+ * Cancelling a close will still leave the stream closed, but some
+ * streams can use a faster close that doesn't block to e.g. check
+ * errors.
+ */
+ g_cancellable_cancel(kill_me_now);
+ idle_server_connection_disconnect_full_async(priv->conn, SERVER_CONNECTION_STATE_REASON_ERROR, kill_me_now, NULL, NULL);
+ g_object_unref(kill_me_now);
+ return FALSE;
+ }
+
+ return TRUE;
+ }
+
+ if (priv->msg_queue->length > 0) {
+ /* No point in sending a PING if we're sending data anyway. */
+ return TRUE;
+ }
+
+ priv->ping_time = now;
+ g_snprintf(cmd, IRC_MSG_MAXLEN + 1, "PING %" G_GINT64_FORMAT, priv->ping_time);
_send_with_priority(conn, cmd, SERVER_CMD_MIN_PRIORITY);
return TRUE;
@@ -1060,6 +1102,18 @@ static IdleParserHandlerResult _ping_handler(IdleParser *parser, IdleParserMessa
return IDLE_PARSER_HANDLER_RESULT_HANDLED;
}
+static IdleParserHandlerResult _pong_handler(IdleParser *parser, IdleParserMessageCode code, GValueArray *args, gpointer user_data) {
+ IdleConnection *conn = IDLE_CONNECTION(user_data);
+ IdleConnectionPrivate *priv = conn->priv;
+
+ /* We could compare ping_time to g_get_real_time() and give some indication
+ * of lag, if we were feeling enthusiastic.
+ */
+ priv->ping_time = 0;
+
+ return IDLE_PARSER_HANDLER_RESULT_HANDLED;
+}
+
static IdleParserHandlerResult _version_privmsg_handler(IdleParser *parser, IdleParserMessageCode code, GValueArray *args, gpointer user_data) {
IdleConnection *conn = IDLE_CONNECTION(user_data);
const gchar *msg = g_value_get_string(g_value_array_get_nth(args, 2));
diff --git a/src/idle-parser.c b/src/idle-parser.c
index 170f299..47ac0e5 100644
--- a/src/idle-parser.c
+++ b/src/idle-parser.c
@@ -85,6 +85,7 @@ static const MessageSpec message_specs[] = {
{"NOTICE", "cIr:", IDLE_PARSER_PREFIXCMD_NOTICE_CHANNEL},
{"NOTICE", "cIc:", IDLE_PARSER_PREFIXCMD_NOTICE_USER},
{"PART", "cIr.", IDLE_PARSER_PREFIXCMD_PART},
+ {"PONG", "IIs:", IDLE_PARSER_PREFIXCMD_PONG},
{"PRIVMSG", "cIr:", IDLE_PARSER_PREFIXCMD_PRIVMSG_CHANNEL},
{"PRIVMSG", "cIc:", IDLE_PARSER_PREFIXCMD_PRIVMSG_USER},
{"QUIT", "cI.", IDLE_PARSER_PREFIXCMD_QUIT},
diff --git a/src/idle-parser.h b/src/idle-parser.h
index a900b00..1216054 100644
--- a/src/idle-parser.h
+++ b/src/idle-parser.h
@@ -60,6 +60,7 @@ typedef enum {
IDLE_PARSER_PREFIXCMD_NOTICE_CHANNEL,
IDLE_PARSER_PREFIXCMD_NOTICE_USER,
IDLE_PARSER_PREFIXCMD_PART,
+ IDLE_PARSER_PREFIXCMD_PONG,
IDLE_PARSER_PREFIXCMD_PRIVMSG_CHANNEL,
IDLE_PARSER_PREFIXCMD_PRIVMSG_USER,
IDLE_PARSER_PREFIXCMD_QUIT,
diff --git a/tests/twisted/connect/ping.py b/tests/twisted/connect/ping.py
index 7d77341..9c1f6ca 100644
--- a/tests/twisted/connect/ping.py
+++ b/tests/twisted/connect/ping.py
@@ -1,9 +1,10 @@
"""
-Test Idle sending PINGs.
+Test Idle sending PINGs and timing out if it doesn't get a reply.
"""
from idletest import exec_test
-from servicetest import assertLength
+from servicetest import assertLength, EventPattern
+import constants as cs
def test(q, bus, conn, stream):
conn.Connect()
@@ -15,8 +16,20 @@ def test(q, bus, conn, stream):
stream.sendMessage('PONG', 'idle.test.server', ':%s' % timestamp,
prefix='idle.test.server')
+ q.expect('stream-PING')
+ # If we don't answer Idle's ping, after some period of time Idle should
+ # give up and close the connection.
+ q.expect_many(
+ EventPattern('irc-disconnected'),
+ EventPattern('dbus-signal', signal='StatusChanged',
+ args=[cs.CONN_STATUS_DISCONNECTED, cs.CSR_NETWORK_ERROR]),
+ )
+
if __name__ == '__main__':
- exec_test(test, params={
+ # We expect Idle to blow up the connection after three intervals without a
+ # reply. So the default 5-second test timeout *should* just be enough, but
+ # let's not risk it.
+ exec_test(test, timeout=10, params={
'keepalive-interval': 1,
})
More information about the telepathy-commits
mailing list