SIP transport and transaction layer

[washort]

RFC 3261 describes a "transaction" layer which handles the SIP transaction state machines. Current SIP code doesn't provide any of that.

[washort]

(In [25701]) Branching to 'sip-transactions-3584'

[washort]

(In [25923]) Branching to 'sip-transactions-3584-2'

[washort]

Ready for review.

[exarkun]

I'll probably have to actually read some of the SIP RFC to give this branch a really good review, but here's some initial comments:

1. computeBranch uses has_key - switch to in please
2. there's some whitespace missing in the get calls in the list comp in computeBranch
3. Would computeBranch make more sense as a method of Message?
4. computeBranch is unused and untested
5. Would responseFromRequest make more sense as a Response.fromRequest class method?
6. responseFromRequest seems to be an exact duplicate of Base.responseFromRequest and Proxy.responseFromRequest (which are duplicates of each other). I guess Base and Proxy are headed for deprecation, though? If so I suppose that's okay.
7. I think the idiom for parameterizing the reactor these days is to make it a parameter to __init__, rather than to have it as an attribute and require it to be set externally (this also means the attribute can be private). This probably means all of the transaction classes should be adjusted slightly.
8. Do all of these transaction objects have a shared interface? Are they objects that anyone is going to want to supply alternate implementations of? If so, it'd be good to have the interface defined. If not, perhaps they can be private to the sip module?
9. Some code in this branch refers to SIPTransport, but that class doesn't exist yet. Perhaps there ought to be an interface?
10. The module would benefit from some high-level overview of how all the pieces fit together. A few sentences in the module docstring would probably ease the process of understanding what's what immensely.

I probably should have said this for the common digest auth branch, but I missed it I guess - test_sip.py shouldn't emit deprecation warnings where it is exercising deprecated SIP APIs (well, no Twisted test module should emit deprecation warnings ;). Can you add suppressions where it makes sense (or file a ticket for doing so)?

[washort]

* Removed computeBranch. * Moved responseFromRequest to Response.fromRequest. * Fixed reactor parameterization as you described. * Transaction objects probably aren't ever something it would be useful to provide alternate implementations of, but code outside this module will need to instantiate them. * Deprecation warning suppressions added for all non-deprecation tests. * SIPTransport, usage examples, and a better docstring will be added in #3616.

[exarkun]

1. twisted/protocols/sip.py
   1. The last line of Response.fromRequest looks like it's probably misindented. I guess there aren't any good tests for this method.
   2. T1, T2, and T4 should be private. Applications shouldn't need to care about them, and maybe we'll want to change the code to make actual estimates for their values (T1 and T4, at least) later on, rather than using fixed constants.
2. twisted/test/test_sip.py
   1. The BasicAuthorizer suppression appears in the module-level suppress list three times. How come?

It's pretty tough to understand most of this code. I hope that's just because the transport is missing. I think you should probably go ahead and put the transport code into this branch so that the whole picture is there. As it is, a lot of the code seems broken or useless (probably because it is without a transport - eg, how are transactions managed? how does a server avoid having them pile up forever, even after they've terminated? in Sine, I see this is taken care of, but there seems to be no handling of it in this code; transactions go to the terminated state and then... nothing else happens).

[exarkun]

Oh. Also, I found the explicit state machine version of this code much easier to read. Twisted probably needs something like epsilon.modal. Maybe until that happens, something like this crappy thing itamar and I wrote would help sip.py, though:

http://twistedmatrix.com/trac/browser/branches/expressive-http-client-886-2/twisted/web/_newclient.py?rev=25913#L651

[exarkun]

(In [26060]) don't punish twisted.web.tap for twisted.web.trp

refs #3584

[exarkun]

Arg. Sorry, that message was intended to go elsewhere.

[washort]

OK, I think the transport code here is reasonably complete, at least enough to answer the questions about transaction lifetime management.

[thijs]

Please update the copyright headers by adding 2009 in the files you modified.

[radix]

1. sip.py:
   1. SIPTransport.tu is a bad attribute name. Also, it looks like it should be private, because it doesn't seem to be used elsewhere? In fact, a lot of the attributes in this class look like they should be private (hosts, port, parser, messages, clock). It seems that, given that you made some of them private, that you intentionally made the non-_ attributes public. Is that true?
2. It seems like there should be an ISIPTransport, since it looks like there may be other implementations of this (stuff like isReliable indicates this). We should have one so it's clear what's meant to be implemented by other parties.
   1. What you have in SIPTransport.isReliable.doc should be on the interface, and SIPTransport.isReliable.doc should say something like "SIPTransport is not reliable. @return: False".
3. computeBranch should use 256 instead of 255 for the range of bytes to serialize to hex.
4. Also you can say randrange(256) instead of randrange(0, 256).
5. In the line response.headers[name] = request.headers.get(name, [])[:] of Response.fromRequest, the "[:]" is untested.
6. In the line response.headers[name] = request.headers.get(name, [])[:] of Response.fromRequest, the ".get()" usage is untested. That is, there is no test which exercises the case in which the key is not in the request headers.
7. In the line return "<SIP Response %d:%s>" % (id(self), self.code) of Response.repr, it's preferable to use something involving "%s" % (hex(twisted.python.util.unsignedID(self)),).
8. The documentation of tu: @ivar tu: an implementor of L{ITransactionUser}., could do with some improvement. For one, it could be encoded into a @type instead of an @ivar. Secondly, it should say "provider" instead of "implementor". The @ivar should also indicate a bit more about what's done to the tu. Like, for example, some general info about which messages are dispatched to it.
9. + @ivar port: The port number this element listens on.. Element?
10. It's easier to read the docstrings when successive lines of epytext fields are indented (with four spaces).
11. It looks like nothing is ever removed from _serverTransactions.
12. The @ivar of _oldStyleServerTransactions is ungrammatical and unclear. It should be a bit more verbose to be a bit more clear.
13. I think that the last two lines of ClientTransaction.init should be a method of SIPTransaction.
14. ClientTransaction.init should be refactored so that all the code which actually *does* the request should be in a different method called by the user. Initializers should just initialize the state.
15. The implementation of SIPTransport.datagramReceived could be greatly simplified by giving the MessagesParser the ability to dispatch its parsed messages immediately to distinct methods for requests and responses.
16. sendRequest's docstring should be rewritten using epytext fields.
17. sendRequest's docstring should indicate that the target should include an IPv4 address and not a hostname.
18. There's a spurious blank line at the beginning of sendRequest.
19. I had a bit of trouble understanding the code in sendRequest. Can you please add a comment referring to the RFC section which this method is implementing?
20. There's a spurious newline at the beginning of _handleRequest
21. 'msg' and 'addr' parameters to _handleRequest should be documented in the docstring.
22. It looks like that a request which contains a hostname instead of an IP address as the 'host' property will not be handled correctly (Section 18.1.1 of RFC 3261 indicates that it is RECOMMENDED for clients to send FQDNs as hostnames instead of IP addresses).
23. in _handleRequest, you're checking if via.rport is True in a via returned from parseViaHeader, but in trunk it seems that the .rport attribute should not be used. I guess this should be changed to use .rportValue. I filed bug #3670 in response to this.

I would appreciate it if when you respond to this review you explicitly indicate the resolution status of the points made.

Okay, I'm really tired and the sprint is nearing an end so I need to cut this review short. I haven't gotten through most of the code. I'll leave this branch in the review state, because I need to continue it, but I'd appreciate it if you could respond to these issues so that I can look at your resolutions when I look at this branch next.

[exarkun]

I'm going to attempt to finish this review.

[exarkun]

24. in _handleRequest, the nested function _badRequest squashes all non-SIPError failures into INTERNAL_ERROR. It should probably be logging them as well, as they likely indicate some kind of bug, yes?
25. also in _handleRequest, the last log.err errback added to the maybeDeferred(requestReceived) doesn't seem to be tested. It would also be nice if it included a 2nd parameter describing where the failure comes from.
26. in _handleResponse, the target parameter seems to be unused.
27. in sendRequest and sendResponse, NotImplementedError should be raised with the one-argument form of raise.
28. ITransactionUser.start's docstring is misformatted
29. ITransactionUser.requestReceived's docstring should use epytext markup for the return value docs
30. ServerTransaction.messageReceivedFromTU uses 100 and 200 instead of TRYING and OK.
31. What happens to the RuntimeError which ServerTransaction.messageReceived and ServerTransaction.messageReceivedFromTU might raise? It looks like they'll just pop up through datagramReceived and get handled by the reactor. This can happen due to input from the network, is this the behavior we want?
32. ServerInviteTransaction's docs for _timerG and _timerH could be a bit better - they're basically just type docs now. If it's possible to say something more than "Timer for the G state of blah blah blah", then it'd be good to do so. Otherwise, a reference to the relevant section of the RFC would be nice.
33. _timerI docs are missing.
34. ServerInviteTransaction.messageReceivedFromTU uses 200, 300, instead of TRYING, and the yet undefined MULTIPLE_CHOICES.
35. ClientTransaction._doTimeout uses 408 instead of TIMEOUT (several more instances of this kind of thing elsewhere in the changeset which I won't bother to point out individually)
36. ClientTransaction's timer attributes could use similar doc help
37. ClientInviteTransaction's timer attributes, too. Isn't there a bunch of boring duplication between all this timeout management? EFK seems a lot like GHI and ABD. Absolute minimum line count definitely isn't the goal, but repeating all of this fiddly timeout stuff two or three times. It seems like, at least in EFK/GHI (and maybe ABD too, but it's shaped slightly differently so I'm not sure), there's:
    1. the retransmit timer
    2. the overall timeout timer
    3. the move-to-terminated timer
38. ClientInviteTransaction._cancelDeferred doesn't seem pointful. It's created, fired, and returned all from the same method. Can it just be a local in cancel?
39. (switching to test_sip.py for the rest of the review) MakeMessageTestCase.test_fromRequest's docstring looks like it ends mid-sentence.
40. TransportTestCase might want to refer to SIPTransport in order to differentiate from the (eventual) TCP transport. Oh, but SIPTransport doesn't have anything in its name which indicates it's UDP. Maybe it should?
41. TransportTestCase.test_newRequestReceived uses assertTrue(len(list), 1) - probably meant to say assertEquals
42. It seems like a lot of tests would be a bit simpler if Message instances could be compared against each other for equality. Then test_newRequestReceived's last 4 lines could be replaced with one assertEquals call.
43. test_oldStyleRequestRetransmissionReceived should not make any assertions inside messageReceived, just collect objects there and assert about them later.
44. test_oldStyleInviteRequestRetransmissionReceived, test_oldStyleAckRequestRetransmissionReceived, test_requestRetransmissionReceived likewise.
45. It looks like these previously mentioned four tests could share TU and StubServerTransaction definitions rather than each re-defining them.
46. in test_handlerFailure, test_errorCode, test_initialTrying, and test_terminated, lambda r: responses.append(r) is just a long way to spell responses.append
47. test_sendRequest, test_sendResponse, etc should use SIPTransport.makeConnection instead of directly setting the transport attribute.
48. the last assertion in test_sendResponse, the one about 'client.com', is probably bogus :( Passing a hostname to udp transport.write is deprecated. This suggests that the implementation will need to do some things with DNS, I guess.
49. test_noReliableTimerG might want to assert that there are no delayed calls at all in the clock; that way a call way past what it expects will still fail the test.
50. test_timerEProceeding doesn't need to set self.tu.responseReceived to anything, the implementation that's there already does what the test wants. Same for test_timerK and test_noReliableTimerK and test_terminated below.
51. test_noReliableTimerE shouldn't make assertions in responseReceived. Likewise for many of the following tests.
52. Overall, please check the spacing between methods and classes; it's not right everywhere. Also, at the last sprint I guess it was decided that assertEquals wins over assertEqual, so please switch the code to that form (including for assertNotEqual etc).
53. Many of the tests could probably benefit from references to the relevant section of the RFC. Adding these annotations to as many of the tests as you can stomach would be great.

Overall, wow. SIP is crazy. Great job.

[washort]

(In [26304]) Branching to 'sip-transactions-3584-3'

[washort]

I think I'm going to respond to all this a few at a time.

1. I've changed the attributes named tu to _transactionUser. Other private attributes made private.
2. ISIPTransport added. Docstrings changed to refer to it.
3. Fixed.
4. Fixed.
5. Test added for that.
6. It's invalid for a SIP message to not have the headers that fromRequest copies, so I've removed the get and added a check in SIPTransport for the presence of required request headers, producing a "400 Bad Request" response if they are absent.
7. Fixed. Also fixed similar code in Request.
8. Changed. Also removed the tu attribute from ServerTransaction and ServerInviteTransaction since nothing uses it.
9. I was confused on terminology. Changed to 'transport'.
10. Indented.

[washort]

1. Indeed not. Added a test for SIPTransport responding to serverTransactionRemoved (which was implemented only on a test stub before!)
2. Rewritten.
3. Yes --
4. Added a send method to ClientTransaction and ClientInviteTransaction, containing that code.
5. Moved some code into _BetterMessagesParser. This probably won't be OK when the TCP transport is added, but it can be fixed then.
6. Done. It refers to ISIPTransport's docstrings now.
7. Done.
8. Fixed.
9. Added.
10. Fixed.
11. Added.

[outcast]

I have been promising glyph I would get to this. Here I go!

[outcast]

patch for items: 23, 28, 30 and some minor code format changes

[outcast]

I did some quick fixes. Just still trying to wrap my head around the code and everything that is going. It is smal, but it is something.