Ticket #5047 enhancement closed fixed

Opened 6 months ago

Last modified 4 months ago

IRCClient heartbeat

Reported by: jonathanj Owned by: jonathanj
Priority: normal Milestone:
Component: words Keywords: irc
Cc: ralphm, thijs Branch: branches/irc-heartbeat-5047-2
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.

Attachments

Change History

1

  Changed 6 months ago by DefaultCC Plugin

  • cc ralphm added

2

  Changed 6 months ago by jonathanj

  • branch set to branches/irc-heartbeat-5047
  • branch_author set to jonathanj

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

3

  Changed 6 months ago by jonathanj

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

4

  Changed 6 months 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.

5

follow-up: ↓ 7   Changed 6 months ago by thijs

  • owner set to jonathanj
  • keywords review removed
  • cc thijs added

Thanks. Minor things:

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

6

  Changed 6 months ago by jonathanj

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

7

in reply to: ↑ 5   Changed 6 months ago by jonathanj

  • owner jonathanj deleted

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.

8

  Changed 6 months ago by jonathanj

  • keywords review added

9

follow-up: ↓ 10   Changed 6 months 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!

10

in reply to: ↑ 9   Changed 6 months ago by jonathanj

  • owner changed from jonathanj to exarkun
  • keywords review added

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.

11

  Changed 6 months ago by exarkun

  • owner exarkun deleted

12

  Changed 5 months ago by jonathanj

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

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

13

  Changed 5 months ago by jonathanj

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

14

  Changed 4 months ago by exarkun

  • owner set to jonathanj
  • keywords review removed
  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.

15

  Changed 4 months ago by jonathanj

  • status changed from new to closed
  • resolution set to fixed

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

Add/Change #5047 (IRCClient heartbeat)


Change Properties
jonathanj
Action
as closed
Next status will be 'reopened'
 
Note: See TracTickets for help on using tickets.