[23:05] glyph: do you have a few minutes to help me decide which of a small group of thread-related changes should stay and which should be reverted? [23:06] exarkun: If it does not involve looking at code, yes :) [23:07] there are three: PosixReactorBase.__init__ does whenThreaded(registerAsIOThread); PosixReactorBase.startRunning does registerAsIOThread(); PosixReactorBase.iterate does registerAsIOThread() [23:07] the middle one has been there for a long time [23:07] the other two were added a week ago [23:07] Hmm. [23:08] I don't think we care that the reactor is created from the same thread it's run in. [23:08] so I'd say the first should go [23:08] btw [23:08] http://lists.xensource.com/archives/html/xen-devel/2005-04/msg00027.html [23:08] Shorter URL: http://alnk.org/realrat [23:08] Also, do you think I am wrong to think that they all need to use whenThreaded? [23:09] dash: My mind cannot comprehend [23:09] exarkun: Hee [23:10] glyph: It's almost enlightening, in a perverse sort of way ;) [23:11] *** glyph checks what whenThreaded does [23:12] exarkun: No, I believe that they definitely should not, at least not with its current implementation. [23:12] :( [23:13] exarkun: it builds up a list of callbacks, so if the iterate() one did it, you'd build up a ridiculously huge list pretty fast [23:13] The iterate() one bothers me somewhat. I hope to remove it. [23:14] But that's merely an efficiency concern. [23:14] I think it would lead to correct results, which the current code does not. [23:14] I think that the iterate() one should be removed too. [23:14] at least, not in the absence of thread support. [23:14] So what if we leave only the __init__ one, with whenThreaded [23:15] I think that preserves the spirit of the oldest one, from startRunning, but makes it actually correct, since tests calling only iterate() will hit __init__ but not startRunning [23:17] what if there was an ifThreaded instead of whenThreaded? [23:18] exarkun: the whenThreaded is silly [23:18] radix: What'd be the advantage? [23:18] glyph: Er? [23:19] glyph: Keep in mind I'm trying to fix both the actual threaded behavior as well as the behavior when "import thread" raises an ImportError. [23:19] whenThreaded strikes me as quite useful for the latter. [23:19] Tests that use iterate *should* use startRunning [23:20] Sigh. [23:20] Iterate is such garbage. [23:20] I never should have added it. [23:20] anyway [23:21] If the reactor is created, then sent off to another thread to be run (as it might be in, say, a wx application like Chandler), then putting it in __init__ would be bad [23:21] er, no? [23:21] Maybe whenThreaded/registerAsIOThread have a surprising interaction [23:21] but it turns out to be right, I think, even when called from __init__ [23:22] exarkun: maybe I misunderstood, but I figured the usage whenThreaded you proposed was to make it so that the calls didn't happen at all if threading isnn't currently enabled [23:22] since it results in whatever thread calls threadable.init(1) to be registered as the IO thread [23:22] _not_ whatever thread called PosixReactorBase.__init__ [23:22] that is the next hing I was going to bring up [23:23] that should always be the actual IO thread, although right now some applications might have been fooled by init()'s docstring and called it [23:23] but we could replace it with a nop and have the reactor call the real implementation [23:24] wait, .init() has to be called from the IO thread? why? [23:24] if you want to depend on whenThreaded(registerAsIOThread) to dtrt, which I am proposing we do [23:25] I do not like whenThreaded [23:25] right now its documentation suggests that any program which uses threads call it [23:25] but that is not accurate [23:25] if you use twisted's threading APIs you don't have to call it ever yourself [23:25] and I doubt anyone ever does [23:26] I agree that the behavior of whenThreaded is freaky [23:27] I am not sure how to replace it, though [23:27] Also a lot of stuff uses it [23:27] whenThreaded is used 3 times in the entire Twisted codebase [23:27] one of the uses is in log.py and is totally stupid [23:27] one is in context.py and could be fixed [23:27] and the third is the one you're talking about [23:27] okay maybe not a lot [23:28] *** glyph googles for 3rd-party code [23:28] heh, I am doing that too :) [23:28] google finds 1 commit message and the API docs for 1.2 and 1.1 [23:28] it can't even find it in the docs for 1.3 :D [23:28] ok [23:28] so how shall I change it [23:29] Well [23:29] I'm thinking [23:30] let's make .iterate() nice and slow, so people won't want to call it [23:30] no [23:30] buildbot takes too long already [23:30] It can do the check to make sure that it's in the IO thread, that it's not attempting reentrancy, and then set itself up as the IO thread if one hasn't been set [23:30] that would be more trouble than it is worth [23:30] I don't think it's actually going to slow down buildbot appreciably [23:30] it might slow down pythondirector if it called .iterate() a lot [23:31] anyway, basically write the 'set up the reactor to run' logic and call it in startRunning and in iterate [23:31] the logic being [23:31] if there's an IO thread already, let's make sure we're in it and we're not already running [23:31] if there isn't an IO thread, make us the IO thread (if we are using threads) [23:32] and yes, threadable.init ought to set the IO thread as well, because it was doing that before anyway and that is really the correct time to do it [23:32] actually [23:32] who calls threadable.init? [23:32] random code and IReactorThreads methods [23:32] _initThreadPool [23:32] nice [23:32] that _is_ the right place to call it, and that _does_ have to be in the IO thread [23:33] yes [23:33] OK, so I was totally wrong about threadable.init() being called from a non-IO thread [23:34] sounds like the right place to do it is *just* in threadable.init() then, leave it out of the reactor entirely [23:34] well, except for "random code" [23:35] ./twisted/conch/ssh/factory.py: threadable.init(1) [23:35] ./twisted/flow/test/test_flow.py:threadable.init() [23:35] ./twisted/test/test_internet.py:threadable.init(1) [23:35] for example. [23:35] unlikely that any of those are in a non-IO thread :) [23:35] unlikely, yes [23:35] but not as satisfyingly absolutely in the IO thread as _initThreadPool [23:35] GAH that import isn't even USED [23:36] and the docstring for threadable.init suggests _call this method_ to anyone reading, imho [23:36] OK, the docstring should clearly be fixed [23:36] I've deleted all three of those calls in my branch, btw [23:36] I just dunno about code outside of twisted [23:36] actually maybe only two of them, I think the test_internet call was important [23:37] seriously>? [23:37] it doesn't use IReactorThreads [23:37] it just makes a thread and does stuff [23:37] augh [23:37] that test is brokeen [23:38] eh [23:38] it is avoiding covering more code than it cares about :) [23:38] You could test the same functionality using _actual_ public APIs [23:38] yes! however [23:38] are we going to say you can't start a thread except using twisted's thread APIs if you are using twisted? [23:38] I think that is what we would have to do if we say you can never call threadable.init [23:43] I think perhaps it would be best to suggest that if you are going to start threads and manage threads, you should do it from a function called from callInThread [23:44] hrm. [23:44] I would agree entirely, if callInThread didn't force you into using a single threadpool. [23:44] again, chandler poses a problem - I bet they're not doing that. [23:46] So, um, to back up and perhaps narrow scope slightly [23:46] The current code does the right thing, I think, except when the platform lacks threads [23:46] it does more than it needs to, probably, but it doesn't look like it ever does anything that's plain wrong [23:47] platforms that lack thread support need to just not have registerAsIOThread called, or registerAsIOThread needs to be a no-op on those platforms [23:47] Hmm [23:48] Correction: registerAsIOThread in __init__ on a thread-supporting platform is also sub-optimal now, as it forces the import of the thread module when we may not actually need it. [23:48] Your earlier proposal to do a crapload of examination in startRunning and iterate seems the most sane to me right now [23:49] However, whenThreaded still seems like the right solution to this problem to me [23:50] So, for example, callFromThread when the reactor is not running will...? [23:50] And maybe dumping it actually does introduce a bug: whenThreaded lets us synchronize blahblahblah right when threads are enabled, whereas checking in iterate is subject to a race [23:51] The obviously correct implementation of isInIOThread (to me) is True iff the thread id matches up _or_ registerAsIOThread has never been called [23:51] but I may be missing something [23:52] exarkun: the problem is that .iterate() is stupid and cannot be made to work [23:52] exarkun: reactor startup is the correct time to set the IO thread [23:52] exarkun: it should be _un_set in reactor teardown [23:52] (though perhaps not in crash()) [23:53] Ok, but that is moot, because I am not going to remove iterate() any time soon [23:54] Yes, but having iterate() behave in random, non-deterministic, broken ways in edge cases is preserving its current behavior :) [23:54] Ok [23:54] So whenThreaded(registerAsIOThread) in startRunning and no calls to registerAsIOThread anywhere else? [23:55] That seems like a good start. [23:55] ok so I am just reverting the entire change of a week ago :)