IRCClient heartbeat

[jonathanj]

IRC servers tend to drop clients that they consider inactive, client sends no data for some period of time, causing the transport to die in a way that may not invoke the usual connectionLost method until it is written to.

A simple implementation could send a PING server.hostname periodically.

[jonathanj]

(In [31623]) Branching to 'irc-heartbeat-5047'

[jonathanj]

(In [31624]) Implement a heartbeat for IRCClient, refs #5047.

[jonathanj]

Use a LoopingCall to send a PING message every 120 seconds, by default, to keep the IRC connection alive. The heartbeat can be disabled by setting IRCClient.heartbeatInterval to None.

[thijs]

Thanks. Minor things:

1. New methods need an @since marker
2. The docstring for heartbeatInterval doesn't mention that the default is 120 seconds.

[jonathanj]

(In [31630]) Add @since markers and improve heartbeatInterval docstring, refs #5047.

[jonathanj]

Replying to thijs:

Thanks. Minor things: 1. New methods need an @since marker 1. The docstring for heartbeatInterval doesn't mention that the default is 120 seconds.

Done.

[exarkun]

1. Yay. I love StringTransport.
2. In the IRCClient class docstring, probably worth mentioning that the hostname attribute is None until the server informs the client of its hostname, and then it is initialized from that server message.
3. In test_irc.py:
   1. assertIdentical is preferred over assertTrue(x is y) and assertNotIdentical over assertTrue(x is not y)
   2. clock.advance(n) can be used instead of clock.pump([n])
   3. If you like, you can also assert about the length of clock.getDelayedCalls(). If it's 0, then all delayed calls must have been cleaned up (this, rather than advancing the clock again and asserting that no more calls were made).
4. A number of existing tests are failing now, some because of left over delayed calls, some because of the connectionLost signature change (which, ugh, what the heck is wrong with Protocol.connectionLost, jeez).

Thanks!

[jonathanj]

2. In the IRCClient class docstring, probably worth mentioning that the hostname attribute is None until the server informs the client of its hostname, and then it is initialized from that server message. 3.1. assertIdentical is preferred over assertTrue(x is y) and assertNotIdentical over assertTrue(x is not y) 3.2. clock.advance(n) can be used instead of clock.pump([n]) 3.3. If you like, you can also assert about the length of clock.getDelayedCalls(). If it's 0, then all delayed calls must have been cleaned up (this, rather than advancing the clock again and asserting that no more calls were made).

Done.

4. A number of existing tests are failing now, some because of left over delayed calls, some because of the connectionLost signature change (which, ugh, what the heck is wrong with Protocol.connectionLost, jeez).

Fixed.

[jonathanj]

(In [31990]) Branching to 'irc-heartbeat-5047-2'

[jonathanj]

(In [31991]) Merge forward and resolve conflicts, refs #5047.

[exarkun]

1. heartbeatInterval is really an ivar not a cvar. Hardly anything is a cvar - basically just stuff that you actually access through the class object directly, or possibly mutable stuff that really gets mutated.
2. CTCPTest.setUp makes self.transport.loseConnection and self.client.connectionLost cleanup actions, but CTCPTest.tearDown still calls them both.
3. The nested _sendHeartbeat definition seems counterproductive. Lose it and self.calls and just observe self.transport.value()? That seems to be a better assertion, since it still covers the 'PING' and it verifies IRCClient._sendHeartbeat is what the looping call calls, instead of manually calling that method in the test method.
4. Similarly, _createHeartbeat could call the original implementation and just the the clock attribute afterwards.

Thanks for working on this. Please merge when you're happy with it and  buildbot is green.

[jonathanj]

(In [32066]) Merge irc-heartbeat-5047-2.

Author: jonathanj Reviewer: exarkun, thijs Fixes: #5047

Implement a heartbeat, a "PING" message to the server at regular intervals, for IRCClient to avoid having the transport die in a way that does not invoke connectionLost until written to.