Opened 12 years ago

Closed 9 years ago

Last modified 9 years ago

#2618 enhancement closed fixed (fixed)

Have Xish STREAM_END_EVENT carry the reason Failure.

Reported by: ralphm Owned by: ralphm
Priority: normal Milestone:
Component: words Keywords:
Cc: ralphm, Adam Branch: branches/xmpp-stream-end-failure-2618
branch-diff, diff-cov, branch-cov, buildbot
Author: awclin

Description

When a stream gets disconnected, the STREAM_END_EVENT event is dispatched. Currently, this event carries the XmlStream instance of the stream that ended. But it would be useful to have access to the reason Failure that is passed to connectionLost that triggers the dispatching of the event.

Attachments (2)

2618.patch (2.4 KB) - added by Adam 9 years ago.
2618-2.patch (6.0 KB) - added by Adam 9 years ago.

Download all attachments as: .zip

Change History (23)

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

Milestone: Words-8.0.0+1Twisted-8.2

Milestone Words-8.0.0+1 deleted

comment:2 Changed 11 years ago by ralphm

Milestone: Twisted-8.2

comment:3 Changed 9 years ago by <automation>

Owner: ralphm deleted

comment:4 Changed 9 years ago by Adam

Cc: Adam added
Keywords: review added

I've wrapped the reason in a Failure, and passed that to the event dispatcher instead of the xmlstream. I don't see the need to maintain passing a reference to the xmlstream itself, as the typical behaviour upon receiving this event will be to instantiate a new one or clear down completely.

Changed 9 years ago by Adam

Attachment: 2618.patch added

comment:5 Changed 9 years ago by ralphm

Owner: set to ralphm
Status: newassigned

comment:6 Changed 9 years ago by ralphm

Keywords: review removed
Owner: changed from ralphm to Adam
Status: assignednew

Thanks for picking this up!

  • Why wrap the failure? What happened (stream ended) is known because of the event name, so you could simply pass reason, as this is a Failure already. I also notice that Wokkel already seems to expect this in some places.
  • I'd like a separate test for this, instead of modifying test_receiveBadXML.
  • Try to keep the first sentence of a docstring within the first line as a summary, you can explain the test after a blank line.
  • Please also modify twisted.words.protocols.jabber.xmlstream.StreamManager so that its _disconnected passes the reason onto connectionLost on all handlers.

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

Also, don't use should in a test method docstring. "X does y." instead of "X should do y."

comment:8 in reply to:  6 Changed 9 years ago by Adam

  • Why wrap the failure? What happened (stream ended) is known because of the event name, so you could simply pass reason, as this is a Failure already. I also notice that Wokkel already seems to expect this in some places.

multitasking/rushing fail on my part. The tests submitted simple strings instead of failure.Failure objects (although of course that doesn't happen in the actual operation of twisted). I've altered the tests to reflect this.

  • I'd like a separate test for this, instead of modifying test_receiveBadXML.

done

  • Try to keep the first sentence of a docstring within the first line as a summary, you can explain the test after a blank line.

done

  • Please also modify twisted.words.protocols.jabber.xmlstream.StreamManager so that its _disconnected passes the reason onto connectionLost on all handlers.

done

@Exarkun - I'd fallen into trap of fitting in with the existing docstring style :) Have updated my docstrings and all others in the file in question to reflect your comment.

@reviewer - The patch is taken from trunk.

Changed 9 years ago by Adam

Attachment: 2618-2.patch added

comment:9 Changed 9 years ago by Adam

Keywords: review added
Owner: Adam deleted

comment:10 Changed 9 years ago by ralphm

Author: ralphm
Branch: branches/xmpp-stream-end-failure-2618

(In [30800]) Branching to 'xmpp-stream-end-failure-2618'

comment:11 Changed 9 years ago by ralphm

Author: ralphmawclin

comment:12 Changed 9 years ago by ralphm

(In [30801]) Apply 2618.patch. Re: #2618.

comment:13 Changed 9 years ago by ralphm

(In [30802]) Apply 2618-2.patch. Re: #2618.

comment:14 Changed 9 years ago by ralphm

(In [30803]) Some minor cleanups. Re: #2618.

comment:15 Changed 9 years ago by ralphm

Owner: set to ralphm
Status: newassigned

comment:16 Changed 9 years ago by ralphm

Keywords: review removed

Aside from some minor cleanups (see [30803]) this is good to go. I'll merge this.

comment:17 Changed 9 years ago by ralphm

(In [30804]) Add missing topfile. Re: #2618.

comment:18 Changed 9 years ago by ralphm

Resolution: fixed
Status: assignedclosed

(In [30805]) Pass reason for connectionLost in XMPP on to observers of STREAM_END_EVENT.

Author: awclin Reviewer: ralphm Fixes: #2618

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

  1. The contents of .misc news fragments are ignored. If you want something to show up in the news file, you need to make the news fragment be some other type.
  2. for XmlStreamTest.connectionLostMsg, I don't think you really want unicode. It probably won't make a difference, but you can't actually have non-ascii in a unicode string passed Exception, so I don't think it helps at all to test with an all-ascii unicode string.
  3. in test_send docstring, Sending data results into it being written seems to be a miswording
  4. in test_streamEnd, these lines look like mistakes:
           self.assertTrue(type(streamEnd[0]), failure.Failure)
           self.assertTrue(streamEnd[0].getErrorMessage, self.connectionLostMsg)
    

You probably want assertIsInstance to replace the former and assertEquals for the latter.

You can fix these by reverting or filing a new ticket, whichever is easier.

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

(Oh, but the only way you can fix the news fragment is by reverting)

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

See #4902

Note: See TracTickets for help on using tickets.