Opened 3 years ago

Closed 3 years ago

#5047 enhancement closed fixed (fixed)

IRCClient heartbeat

Reported by: jonathanj Owned by: jonathanj
Priority: normal Milestone:
Component: words Keywords: irc
Cc: ralphm, thijs Branch: branches/irc-heartbeat-5047-2
(diff, github, buildbot, log)
Author: jonathanj Launchpad Bug:

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 3 years ago by DefaultCC Plugin

  • Cc ralphm added

comment:2 Changed 3 years ago by jonathanj

  • Author set to jonathanj
  • Branch set to branches/irc-heartbeat-5047

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

comment:3 Changed 3 years ago by jonathanj

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

comment:4 Changed 3 years ago by jonathanj

  • Keywords review added
  • Owner jonathanj 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 follow-up: Changed 3 years ago by thijs

  • Cc thijs added
  • Keywords review removed
  • Owner set to jonathanj

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 3 years ago by jonathanj

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

comment:7 in reply to: ↑ 5 Changed 3 years ago by jonathanj

  • Owner jonathanj 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 3 years ago by jonathanj

  • Keywords review added

comment:9 follow-up: Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to jonathanj
  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 3 years ago by jonathanj

  • Keywords review added
  • Owner changed from jonathanj to exarkun
  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 3 years ago by exarkun

  • Owner exarkun deleted

comment:12 Changed 3 years ago by jonathanj

  • Branch changed from branches/irc-heartbeat-5047 to branches/irc-heartbeat-5047-2

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

comment:13 Changed 3 years ago by jonathanj

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

comment:14 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to jonathanj
  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 3 years ago by jonathanj

  • Resolution set to fixed
  • Status changed from new to closed

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