Opened 14 years ago
Last modified 10 years ago
#3584 enhancement assigned
SIP transport and transaction layer
Reported by: | washort | Owned by: | outcast |
---|---|---|---|
Priority: | highest | Milestone: | |
Component: | core | Keywords: | |
Cc: | Thijs Triemstra | Branch: |
branches/sip-transactions-3584-3
branch-diff, diff-cov, branch-cov, buildbot |
Author: | washort |
Description (last modified by )
RFC 3261 describes a "transaction" layer which handles the SIP transaction state machines. Current SIP code doesn't provide any of that.
Attachments (1)
Change History (26)
comment:1 Changed 14 years ago by
Author: | → washort |
---|---|
Branch: | → branches/sip-transactions-3584 |
comment:2 Changed 13 years ago by
Branch: | branches/sip-transactions-3584 → branches/sip-transactions-3584-2 |
---|
(In [25923]) Branching to 'sip-transactions-3584-2'
comment:3 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | washort deleted |
Priority: | normal → highest |
Ready for review.
comment:4 Changed 13 years ago by
Description: | modified (diff) |
---|---|
Summary: | SIP transport layer and transaction layer → SIP transaction layer |
comment:5 Changed 13 years ago by
Owner: | set to Jean-Paul Calderone |
---|---|
Status: | new → assigned |
comment:6 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to washort |
Status: | assigned → new |
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:
computeBranch
useshas_key
- switch toin
please- there's some whitespace missing in the get calls in the list comp in
computeBranch
- Would
computeBranch
make more sense as a method ofMessage
? computeBranch
is unused and untested- Would
responseFromRequest
make more sense as aResponse.fromRequest
class method? responseFromRequest
seems to be an exact duplicate ofBase.responseFromRequest
andProxy.responseFromRequest
(which are duplicates of each other). I guessBase
andProxy
are headed for deprecation, though? If so I suppose that's okay.- 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. - 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?
- Some code in this branch refers to
SIPTransport
, but that class doesn't exist yet. Perhaps there ought to be an interface? - 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)?
comment:7 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | washort deleted |
- Removed
computeBranch
. - Moved
responseFromRequest
toResponse.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.
comment:8 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | set to washort |
- twisted/protocols/sip.py
- The last line of
Response.fromRequest
looks like it's probably misindented. I guess there aren't any good tests for this method. T1
,T2
, andT4
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.
- The last line of
- twisted/test/test_sip.py
- The BasicAuthorizer suppression appears in the module-level
suppress
list three times. How come?
- The BasicAuthorizer suppression appears in the module-level
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).
comment:9 Changed 13 years ago by
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:
comment:10 Changed 13 years ago by
comment:12 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | washort deleted |
OK, I think the transport code here is reasonably complete, at least enough to answer the questions about transaction lifetime management.
comment:13 Changed 13 years ago by
Summary: | SIP transaction layer → SIP transport and transaction layer |
---|
comment:14 Changed 13 years ago by
Cc: | Thijs Triemstra added |
---|---|
Keywords: | review removed |
Owner: | set to washort |
Please update the copyright headers by adding 2009 in the files you modified.
comment:15 Changed 13 years ago by
Keywords: | review added |
---|---|
Owner: | washort deleted |
comment:16 Changed 13 years ago by
Owner: | set to radix |
---|---|
Status: | new → assigned |
comment:17 Changed 13 years ago by
- sip.py:
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?
- 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.
- 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".
- computeBranch should use 256 instead of 255 for the range of bytes to serialize to hex.
- Also you can say randrange(256) instead of randrange(0, 256).
- In the line
response.headers[name] = request.headers.get(name, [])[:]
of Response.fromRequest, the "[:]" is untested. - 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. - 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)),). - 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. + @ivar port: The port number this element listens on.
. Element?- It's easier to read the docstrings when successive lines of epytext fields are indented (with four spaces).
- It looks like nothing is ever removed from _serverTransactions.
- The @ivar of _oldStyleServerTransactions is ungrammatical and unclear. It should be a bit more verbose to be a bit more clear.
- I think that the last two lines of ClientTransaction.init should be a method of SIPTransaction.
- 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.
- 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.
- sendRequest's docstring should be rewritten using epytext fields.
- sendRequest's docstring should indicate that the target should include an IPv4 address and not a hostname.
- There's a spurious blank line at the beginning of sendRequest.
- 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?
- There's a spurious newline at the beginning of _handleRequest
- 'msg' and 'addr' parameters to _handleRequest should be documented in the docstring.
- 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).
- 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.
comment:18 Changed 13 years ago by
Owner: | changed from radix to Jean-Paul Calderone |
---|---|
Status: | assigned → new |
I'm going to attempt to finish this review.
comment:19 Changed 13 years ago by
Keywords: | review removed |
---|---|
Owner: | changed from Jean-Paul Calderone to washort |
- 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?
- 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.
- in _handleResponse, the target parameter seems to be unused.
- in sendRequest and sendResponse,
NotImplementedError
should be raised with the one-argument form ofraise
. ITransactionUser.start
's docstring is misformattedITransactionUser.requestReceived
's docstring should use epytext markup for the return value docsServerTransaction.messageReceivedFromTU
uses100
and200
instead ofTRYING
andOK
.- What happens to the
RuntimeError
whichServerTransaction.messageReceived
andServerTransaction.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? 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._timerI
docs are missing.ServerInviteTransaction.messageReceivedFromTU
uses200
,300
, instead ofTRYING
, and the yet undefinedMULTIPLE_CHOICES
.ClientTransaction._doTimeout
uses408
instead ofTIMEOUT
(several more instances of this kind of thing elsewhere in the changeset which I won't bother to point out individually)ClientTransaction
's timer attributes could use similar doc helpClientInviteTransaction
'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:- the retransmit timer
- the overall timeout timer
- the move-to-terminated timer
ClientInviteTransaction._cancelDeferred
doesn't seem pointful. It's created, fired, and returned all from the same method. Can it just be a local incancel
?- (switching to test_sip.py for the rest of the review)
MakeMessageTestCase.test_fromRequest
's docstring looks like it ends mid-sentence. TransportTestCase
might want to refer toSIPTransport
in order to differentiate from the (eventual) TCP transport. Oh, butSIPTransport
doesn't have anything in its name which indicates it's UDP. Maybe it should?TransportTestCase.test_newRequestReceived
usesassertTrue(len(list), 1)
- probably meant to sayassertEquals
- It seems like a lot of tests would be a bit simpler if
Message
instances could be compared against each other for equality. Thentest_newRequestReceived
's last 4 lines could be replaced with oneassertEquals
call. test_oldStyleRequestRetransmissionReceived
should not make any assertions insidemessageReceived
, just collect objects there and assert about them later.test_oldStyleInviteRequestRetransmissionReceived
,test_oldStyleAckRequestRetransmissionReceived
,test_requestRetransmissionReceived
likewise.- It looks like these previously mentioned four tests could share
TU
andStubServerTransaction
definitions rather than each re-defining them. - in
test_handlerFailure
,test_errorCode
,test_initialTrying
, andtest_terminated
,lambda r: responses.append(r)
is just a long way to spellresponses.append
test_sendRequest
,test_sendResponse
, etc should useSIPTransport.makeConnection
instead of directly setting thetransport
attribute.- 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. 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.test_timerEProceeding
doesn't need to setself.tu.responseReceived
to anything, the implementation that's there already does what the test wants. Same fortest_timerK
andtest_noReliableTimerK
andtest_terminated
below.test_noReliableTimerE
shouldn't make assertions inresponseReceived
. Likewise for many of the following tests.- 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 overassertEqual
, so please switch the code to that form (including forassertNotEqual
etc). - 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.
comment:20 Changed 13 years ago by
Branch: | branches/sip-transactions-3584-2 → branches/sip-transactions-3584-3 |
---|
(In [26304]) Branching to 'sip-transactions-3584-3'
comment:21 Changed 13 years ago by
I think I'm going to respond to all this a few at a time.
- I've changed the attributes named
tu
to_transactionUser
. Other private attributes made private. ISIPTransport
added. Docstrings changed to refer to it.- Fixed.
- Fixed.
- Test added for that.
- It's invalid for a SIP message to not have the headers that
fromRequest
copies, so I've removed theget
and added a check inSIPTransport
for the presence of required request headers, producing a "400 Bad Request" response if they are absent. - Fixed. Also fixed similar code in
Request
. - Changed. Also removed the
tu
attribute fromServerTransaction
andServerInviteTransaction
since nothing uses it. - I was confused on terminology. Changed to 'transport'.
- Indented.
comment:22 Changed 13 years ago by
- Indeed not. Added a test for
SIPTransport
responding toserverTransactionRemoved
(which was implemented only on a test stub before!) - Rewritten.
- Yes --
- Added a
send
method toClientTransaction
andClientInviteTransaction
, containing that code. - Moved some code into
_BetterMessagesParser
. This probably won't be OK when the TCP transport is added, but it can be fixed then. - Done. It refers to
ISIPTransport
's docstrings now. - Done.
- Fixed.
- Added.
- Fixed.
- Added.
comment:23 Changed 10 years ago by
Owner: | changed from washort to outcast |
---|---|
Status: | new → assigned |
Changed 10 years ago by
Attachment: | sip-outcast-03-21-2012.patch added |
---|
patch for items: 23, 28, 30 and some minor code format changes
comment:25 Changed 10 years ago by
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.
(In [25701]) Branching to 'sip-transactions-3584'