Opened 6 years ago

Closed 6 years ago

#5047 enhancement closed fixed (fixed)

IRCClient heartbeat

Reported by: Jonathan Jacobs Owned by: Jonathan Jacobs
Priority: normal Milestone:
Component: words Keywords: irc
Cc: ralphm, Thijs Triemstra Branch: branches/irc-heartbeat-5047-2
branch-diff, diff-cov, branch-cov, buildbot
Author: jonathanj

Description

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.

Change History (15)

comment:1 Changed 6 years ago by DefaultCC Plugin

Cc: ralphm added

comment:2 Changed 6 years ago by Jonathan Jacobs

Author: jonathanj
Branch: branches/irc-heartbeat-5047

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

comment:3 Changed 6 years ago by Jonathan Jacobs

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

comment:4 Changed 6 years ago by Jonathan Jacobs

Keywords: review added
Owner: Jonathan Jacobs deleted

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.

comment:5 Changed 6 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: review removed
Owner: set to Jonathan Jacobs

Thanks. Minor things:

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

comment:6 Changed 6 years ago by Jonathan Jacobs

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

comment:7 in reply to:  5 Changed 6 years ago by Jonathan Jacobs

Owner: Jonathan Jacobs deleted

Replying to 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.

Done.

comment:8 Changed 6 years ago by Jonathan Jacobs

Keywords: review added

comment:9 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jonathan Jacobs
  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!

comment:10 in reply to:  9 Changed 6 years ago by Jonathan Jacobs

Keywords: review added
Owner: changed from Jonathan Jacobs to Jean-Paul Calderone
  1. 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.

  1. 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.

comment:11 Changed 6 years ago by Jean-Paul Calderone

Owner: Jean-Paul Calderone deleted

comment:12 Changed 6 years ago by Jonathan Jacobs

Branch: branches/irc-heartbeat-5047branches/irc-heartbeat-5047-2

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

comment:13 Changed 6 years ago by Jonathan Jacobs

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

comment:14 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Jonathan Jacobs
  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.

comment:15 Changed 6 years ago by Jonathan Jacobs

Resolution: fixed
Status: newclosed

(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.

Note: See TracTickets for help on using tickets.