Opened 10 years ago

Closed 8 years ago

#2385 defect closed fixed (fixed)

twisted.words irc server does not work with gaim

Reported by: Glyph Owned by: David Sturgis
Priority: normal Milestone:
Component: words Keywords:
Cc: Branch: branches/pidgin-2385-3
branch-diff, diff-cov, branch-cov, buildbot
Author: tenth

Description

When I attempt to connect, regardless of whether I specify a password or not, gaim hangs forever and doesn't consider me logged in.

Change History (18)

comment:1 Changed 10 years ago by Jean-Paul Calderone

I checked with ethereal. The login seems to be succeeding but the Twisted Words server doesn't send very many codes in its greeting. I suppose gaim might be waiting for something which it doesn't send before considering login complete.

comment:2 Changed 9 years ago by David Sturgis

Type: enhancementdefect

Itamar and I discovered that including the MOTD lines:

("375", ":- %(serviceName)s Message of the Day - "), ("372", ":- The message of the day is: THERE IS NO MESSAGE OF THE DAY. - "), ("376", ":End of /MOTD command.")

as part of twisted.words.service.IRCUser._welcomeMessages, immediately after the RPL_MYINFO line, is consistent with FreeNode server behavior, and is more acceptable to GAIM.

comment:3 Changed 9 years ago by David Sturgis

Actually, you don't even need the 372 (actual message of the day) line, just 375 (RPL_MOTDSTART) and then 376 (RPL_ENDOFMOTD). hooray.

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

Keywords: easy added

Awesome, thanks for the investigation. Now it should be easy for someone to adjust IRCUser to send these codes and make gaim/pidgin work.

comment:5 Changed 9 years ago by David Sturgis

Author: tenth
Branch: branches/pidgin-2385

(In [25256]) Branching to 'pidgin-2385'

comment:6 Changed 9 years ago by David Sturgis

Keywords: review added; easy removed
Owner: Jean-Paul Calderone deleted

comment:7 Changed 9 years ago by therve

Keywords: review removed
Owner: set to David Sturgis

Here we go:

1.

-            "serviceName": self.hostname,
+            "serviceName": "test",

Why's this change?

2.

+        self.name = nickname;

Please remove the semi-colon.

  1. One flake:
    twisted/words/test/test_irc_service.py:8: 'time' imported but unused
    

Thanks!

comment:8 Changed 8 years ago by David Sturgis

Keywords: review added
Owner: David Sturgis deleted

Itamar and I addressed Therve's review comments, and also added an actual test for Logins. As part of this, we also fixed some bugs found by the test (hostname is always "undefined", fake creation time, etc.)

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

Owner: set to Jean-Paul Calderone
Status: newassigned

Looking good. A few simple things:

comment:10 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jean-Paul Calderone to David Sturgis
Status: assignednew

Let me try that again.

First, some simple things:

  1. In service.py, the new serverInfo attribute of the factory needs to be documented. In particular, all the keys (and their associated values) which it will have need documentation.
  2. In test_service.py, the _assertGreeting method's new docstring has some trailing whitespace in it; it also documents AssertionError as an exception which may be raised, but I think it's actually twisted.trial.unittest.FailTest, or maybe self.failureException.
  3. Further down in that file, the new messageType parameter to _response is undocumented.
  4. testNickServLogin's docstring also has trailing whitespace. As long as you're modifying the function, you should rename it to comply with the naming convention - test_nickServLogin.
  5. In test_irc_service.py
    1. numerous lines end in trailing whitespace
    2. methods should be separated by two blank lines; many are, but setUp and the first test method following it are separated by only one blank line.
    3. The dates in the copyright statement should be made accurate - 2009, not 2001-2008.

Next. The behavior actually being implemented is very bizarre. Now the server sends the MOTD (except there's never an MOTD so it just sends two lines about how there's not an MOTD) whenever it receives NICK. As a separate bug, the server allows multiple NICKs to be issued. This might be a reasonable thing to allow, but it's not being handled properly now. Please file a new ticket for dealing with that misbehavior.

So, anyway. This isn't quite the behavior of other IRC servers (at least, not freenode). Freenode won't send anything until it gets both USER and NICK. Then it sends the whole "greeting" sequence. Then it uses NickServ to challenge you for a password. It would probably be best to emulate this more closely.

Finally, I don't really understand a lot of the changes to service.py. The name attribute, at least, seems necessary to make the test pass (although it wasn't before - I didn't bother to figure out why, but I'm annoyed that it's not obvious :/). However, why are nickname and realm being handled differently now? realm in particular is actually part of IUser and should probably remain that way unless there's a good reason to change it.

comment:11 Changed 8 years ago by David Sturgis

Keywords: review added
Owner: David Sturgis deleted

Itamar and I have made the suggested docstring and whitespace fixes. The new ticket #3664 addresses the MOTD ordering issue. As for nickname and realm, they were required previously, but not set until after login; This new order seems to make more sense. (For example, error messages early in the login process will now contain the correct hostname.)

comment:12 Changed 8 years ago by z3p

Keywords: review removed
Owner: set to David Sturgis

Conflict when merging w/ trunk.

comment:13 Changed 8 years ago by David Sturgis

Branch: branches/pidgin-2385branches/pidgin-2385-2

(In [26276]) Branching to 'pidgin-2385-2'

comment:14 Changed 8 years ago by David Sturgis

Branch: branches/pidgin-2385-2branches/pidgin-2385-3

(In [26277]) Branching to 'pidgin-2385-3'

comment:15 Changed 8 years ago by David Sturgis

Keywords: review added
Owner: David Sturgis deleted

Merged forward (hopefully correctly this time!)

comment:16 Changed 8 years ago by Glyph

Keywords: review removed
Owner: set to David Sturgis

Very minor changes:

  1. Since the structure of serverInfo is a bit silly (it's a dictionary, but it needs to have specific keys, it should really be a collection of attributes) it should be private pending some better structure that we want to publicly expose. In other words, it should start with a single underscore, _serverInfo.
  2. A couple of blank lines have been incorrectly (I assume accidentally?) deleted which aren't really relevant to the implementation, and should be put back in. Merged to trunk, these lines are: service.py:207 service.py:350 service.py:357 service.py:888
  3. There is another problem, for which there should be a separate ticket. AWAY isn't implemented on the server, so when I attempt to log in, I see this (thankfully, apparently harmless) traceback:
    2009-02-22 19:26:06-0500 [IRCUser,0,127.0.0.1] Unhandled Error
    	Traceback (most recent call last):
    	  File "/home/glyph/Projects/Twisted/trunk/twisted/python/context.py", line 37, in callWithContext
    	    return func(*args,**kw)
    	  File "/home/glyph/Projects/Twisted/trunk/twisted/internet/selectreactor.py", line 146, in _doReadOrWrite
    	    why = getattr(selectable, method)()
    	  File "/home/glyph/Projects/Twisted/trunk/twisted/internet/tcp.py", line 459, in doRead
    	    return self.protocol.dataReceived(data)
    	  File "/home/glyph/Projects/Twisted/trunk/twisted/words/protocols/irc.py", line 172, in dataReceived
    	    self.handleCommand(command, prefix, params)
    	--- <exception caught here> ---
    	  File "/home/glyph/Projects/Twisted/trunk/twisted/words/protocols/irc.py", line 184, in handleCommand
    	    self.irc_unknown(prefix, command, params)
    	  File "/home/glyph/Projects/Twisted/trunk/twisted/words/protocols/irc.py", line 191, in irc_unknown
    	    raise NotImplementedError(command, prefix, params)
    	exceptions.NotImplementedError: ('AWAY', '', [])
    

Other than that it looks good! Please fix and merge.

comment:17 Changed 8 years ago by David Sturgis

serverInfo has been marked private with an underscore, and the blank lines have been unremoved to clean up the diff in r26279. I have entered the AWAY implementation issue as ticket #3668.

comment:18 Changed 8 years ago by David Sturgis

Resolution: fixed
Status: newclosed

(In [26281]) Make twisted.words IRC server compatible with Pidgin.

author: itamar, tenth

reviewer: therve, exarkun, z3p, glyph

Fixes #2385

While there are other issues to be resolved with MOTD and AWAY behaviors as per #3664 and #3668 respectively, twisted.words now always sends an MOTD message, which is required for Pidgin to successfully connect to it. Also, the "nickname" and "realm" attributes are now initialized in time for them to appear in error messages during login.

Note: See TracTickets for help on using tickets.