[Twisted-Python] CFReactor
Itamar just added half-closing support to Twisted, and I foolishly volunteered to look at adding support to cfreactor for him. However, that was before I looked at it. Now that I have, I have little idea what it's actually doing, so I'm afraid that I'm unable to actually implement this change. The "SelectableSocketWrapper" looks like a horrible kludge, and I'm not sure why it's necessary. Why does it override the wrapped object's "connectionLost"? Why do you have to wrap&override Selectable.start/stopReading/Writing, instead of doing that stuff in reactor.add/removeReader/Writer? What is simulate?? Essentially, I just don't understand why it is so different from the other reactors. Either hints or else a working half-close implementation for cfreactor would be appreciated. James
On Oct 25, 2004, at 17:24, James Y Knight wrote:
The SSW kludge is necessary because it inherits a lot of functionality from the existing default select-based reactor and its sockets. It is a workaround for the lack of extensibility in the implementation of those sockets and that reactor. It could be less hacky if it were inverted, but then you'd have to subclass every possible port and add the reactor notifications, and essentially copy about 80% of twisted.internet.default instead of subclassing it. If you want to refactor it this way, be my guest.. but I'm not bored enough to do it :) The reason for the loseConnection hack is that the existing implementation of loseConnection doesn't do removeReader/Writer! loseConnection effectively kills the socket without notifying the reactor at all. If the existing implementation told the reactor when it was done with a socket, SSW wouldn't be so much of a kludge.
Either hints or else a working half-close implementation for cfreactor would be appreciated.
You'll probably have to implement the same style of hack that is used for loseConnection. Replace the half-lose-connection method (if it exists) with a method that notifies the reactor of the intention, and then call the original implementation. -bob
On Oct 25, 2004, at 6:02 PM, Bob Ippolito wrote:
Er, I don't believe you. If that was the case, it would be a huge fd leak, which I'm pretty sure Twisted doesn't have. I removed that hack and I don't think anything further broke. Anyways, I've changed cfreactor to do what I think should be the right thing, but it's randomly failing tests. Before my changes it was failing a lot of tests too, so I don't know if this is a regression or not. James
On Oct 26, 2004, at 13:04, James Y Knight wrote:
No. It's not a fd leak because loseConnection closes the file descriptors. It just doesn't tell the reactor it has done so. It should leak SSW instances (memory) now, though.
I don't really have the time or need to debug this anytime soon. When I first wrote CFReactor, it passed all of the tests. Either changes in PyObjC (unlikely) or Twisted (likely) caused these test failures. Some of it is probably due to tests making incorrect assumptions, especially because CFReactor isn't designed to be iterated (it has to set up a timer to stop itself). -bob
On Oct 26, 2004, at 1:35 PM, Bob Ippolito wrote:
Those are only kept in a weakref dict, and in the readers/writers dicts. removeReader/removeWriter look to me like they are being called, so I think it should go away. I dunno if that's a change since cfreactor was written or if I'm just wrong or what.
Me neither. If no one is actually interested, I guess it should just be marked as probably broken and left at that. I'm going to give up on it.
I can't even get the tests to run to completion, they hang on twisted.mail.test.test_smtp.LoopbackESMTPTestCase.testMessages. Since this reactor isn't being run on the buildbot for some reason, it's hard to know how long it's been failing. I also tried checking out revisions 9056 from 2003-10-20 and 9286 from 2003-11-11. Those are failing about a bazillion tests, mostly with pendingTimedCalls still pending: <DelayedCall 35813496 [-0.0020911693573s] called=0 cancelled=0 CFReactor._continueSystemEvent('startup',)> warnings. I dunno what's up with that. James
On Oct 25, 2004, at 17:24, James Y Knight wrote:
The SSW kludge is necessary because it inherits a lot of functionality from the existing default select-based reactor and its sockets. It is a workaround for the lack of extensibility in the implementation of those sockets and that reactor. It could be less hacky if it were inverted, but then you'd have to subclass every possible port and add the reactor notifications, and essentially copy about 80% of twisted.internet.default instead of subclassing it. If you want to refactor it this way, be my guest.. but I'm not bored enough to do it :) The reason for the loseConnection hack is that the existing implementation of loseConnection doesn't do removeReader/Writer! loseConnection effectively kills the socket without notifying the reactor at all. If the existing implementation told the reactor when it was done with a socket, SSW wouldn't be so much of a kludge.
Either hints or else a working half-close implementation for cfreactor would be appreciated.
You'll probably have to implement the same style of hack that is used for loseConnection. Replace the half-lose-connection method (if it exists) with a method that notifies the reactor of the intention, and then call the original implementation. -bob
On Oct 25, 2004, at 6:02 PM, Bob Ippolito wrote:
Er, I don't believe you. If that was the case, it would be a huge fd leak, which I'm pretty sure Twisted doesn't have. I removed that hack and I don't think anything further broke. Anyways, I've changed cfreactor to do what I think should be the right thing, but it's randomly failing tests. Before my changes it was failing a lot of tests too, so I don't know if this is a regression or not. James
On Oct 26, 2004, at 13:04, James Y Knight wrote:
No. It's not a fd leak because loseConnection closes the file descriptors. It just doesn't tell the reactor it has done so. It should leak SSW instances (memory) now, though.
I don't really have the time or need to debug this anytime soon. When I first wrote CFReactor, it passed all of the tests. Either changes in PyObjC (unlikely) or Twisted (likely) caused these test failures. Some of it is probably due to tests making incorrect assumptions, especially because CFReactor isn't designed to be iterated (it has to set up a timer to stop itself). -bob
On Oct 26, 2004, at 1:35 PM, Bob Ippolito wrote:
Those are only kept in a weakref dict, and in the readers/writers dicts. removeReader/removeWriter look to me like they are being called, so I think it should go away. I dunno if that's a change since cfreactor was written or if I'm just wrong or what.
Me neither. If no one is actually interested, I guess it should just be marked as probably broken and left at that. I'm going to give up on it.
I can't even get the tests to run to completion, they hang on twisted.mail.test.test_smtp.LoopbackESMTPTestCase.testMessages. Since this reactor isn't being run on the buildbot for some reason, it's hard to know how long it's been failing. I also tried checking out revisions 9056 from 2003-10-20 and 9286 from 2003-11-11. Those are failing about a bazillion tests, mostly with pendingTimedCalls still pending: <DelayedCall 35813496 [-0.0020911693573s] called=0 cancelled=0 CFReactor._continueSystemEvent('startup',)> warnings. I dunno what's up with that. James
participants (2)
-
Bob Ippolito -
James Y Knight