Opened 3 years ago

Closed 3 years ago

#5176 enhancement closed fixed (fixed)

IRCClient.msg loses data when splitting long messages

Reported by: jonathanj Owned by: jonathanj
Priority: normal Milestone:
Component: words Keywords: irc
Cc: ralphm, firxen@… Branch: branches/fix-ircclient-msg-split-5176-2
(diff, github, buildbot, log)
Author: jonathanj Launchpad Bug:

Description

The length calculations used for splitting long messages don't take into account that when a message like PRIVMSG #foo <long_message> is relayed to other clients by the server it looks something like :nickname!user@host PRIVMSG #foo :<long_message> which is truncated at 510 characters. The current behaviour means that <long_message> loses an unspecified number of characters around the message limit mark for each line.

I think we have enough information to guess a safe maximum:

  • Maximum nickname length can be determined from the ISUPPORT info (using our own current nickname may not always be correct.)
  • Maximum username length seems to be something like 10 characters, I can't find any information on this limit.
  • According to RFC 2812 the maximum hostname is 63 characters (our hostname can be changed on the server side at any point.)

Change History (14)

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/fix-ircclient-msg-split-5176

(In [32230]) Branching to 'fix-ircclient-msg-split-5176'

comment:3 Changed 3 years ago by jonathanj

(In [32231]) Docstrings and cosmetics for twisted.words.test.test_irc.ClientMsgTests, refs #5176.

comment:4 Changed 3 years ago by jonathanj

(In [32232]) Fix what appears to be a copy-paste error, refs #5176.

comment:5 Changed 3 years ago by jonathanj

(In [32234]) Write better tests for IRCClient.msg splitting behaviour, refs #5176.

comment:6 Changed 3 years ago by jonathanj

(In [32235]) Better maximum command length calculation to make failing tests pass, refs #5176.

comment:7 Changed 3 years ago by jonathanj

  • Keywords review added

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

  • Cc firxen@… added
  • Keywords review removed
  • Owner set to jonathanj

Disclaimer: This is my first actual review, so yell if I'm doing it wrong.

(There's a merge conflict with current trunk r32338 (#4990 -- assertEquals->assertEqual), so I used the version from the branch which includes the same changes.)

Why do we use the generic maximum value for the command prefix (which is our own nick!user@host) instead of calculating the actual length? Do we not have reliable access to that?

Apart from those, I don't see any problems with merging the branch.

comment:10 Changed 3 years ago by jonathanj

  • Branch changed from branches/fix-ircclient-msg-split-5176 to branches/fix-ircclient-msg-split-5176-2

(In [32375]) Branching to 'fix-ircclient-msg-split-5176-2'

comment:11 in reply to: ↑ 9 Changed 3 years ago by jonathanj

  • Keywords review added

Replying to jerith:

(There's a merge conflict with current trunk r32338 (#4990 -- assertEquals->assertEqual), so I used the version from the branch which includes the same changes.)

Fixed.

Why do we use the generic maximum value for the command prefix (which is our own nick!user@host) instead of calculating the actual length? Do we not have reliable access to that?

Yes, it is not reliable enough. I've expanded the docstring for _safeMaximumLength to better explain this.

comment:12 Changed 3 years ago by jonathanj

  • Owner jonathanj deleted

comment:13 Changed 3 years ago by jerith

  • Keywords review removed
  • Owner set to jonathanj

Looks good for merging.

comment:14 Changed 3 years ago by jonathanj

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

(In [32380]) Merge fix-ircclient-msg-split-5176-2.

Author: jonathanj
Reviewer: jerith
Fixes: #5176

Be more conservative when determining message length in IRCClient.msg in order to reduce the chance of relayed IRC messages being truncated.

Note: See TracTickets for help on using tickets.