380078 - tinderbox poller should be asynchronous

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Axel Hecht [:Pike]]

Attachment: move synchronous urllib2 to twistd protocol

... and, hrm, call the site just once? Not sure if urlopen actually hits the site, I assume it does. Anyway, I moved the tinderbox parser over to a protocol impl, so things are nicely async with this patch.

I'm not exactly sure if I did the error callbacks right and at the right places, I'm pretty confident though that I had to move the result callback away from the deferred, as that's firing on openConnection, afaict.

Comment 1

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 264139 [details] [diff] [review]
move synchronous urllib2 to twistd protocol

why? What is this solving?

I'm reluctant to take this as I have a couple of systems that are working well now with the current setup and this seems to be a change for the sake of coding. Also, if we're moving to an asynchronous model, it's going to make the state-watching system of the current poller that much harder to constrain.

Comment 2

[Axel Hecht [:Pike]]

Attachment: patch using web.getPage

Luckily, robcee didn't like the previous patch.

Using web.getPage makes the patch much nicer, and actually adds the right deferred to add hooks to.

To completely reply to Rob's comment, this is not a fix for fix' sake, IMHO, the double urlopen in the original implementation is a good reason to not take tinderboxpoller upstream as it currently is.

Comment 3

[Axel Hecht [:Pike]]

Rob, should we add a timeout to getPage, too?

Comment 4

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 264280 [details] [diff] [review]
patch using web.getPage

could you revisit this after checking out the latest version of tinderboxpoller.py? The base changed in bug 373653. Does getPage have a default timeout? If not, we could set one as you suggest.

Comment 5

[Axel Hecht [:Pike]]

Attachment: update to tip, timeout

Copying comments from bug 373653, where this patch ended up first, due to me being a stupid ass.

Updated the patch to tip, and added a timeout.

I used self.pollInterval, which is probably the right order of magnitude, but
I'm not sure if that's going to time out shortly after the new timeout would
fire, and thus skip a query. I could use self.pollInterval/2 or something
instead, not sure. Rob?

Comment #7 [reply] Rob Campbell [:rc] (robcee@)  2007-06-21 07:27:19 PDT

yeah, there will probably be a boundary case where we get a timeout that's just
under 2x the pollInterval. That's pretty common in asynchronous systems. I'd
leave it as it is let the client of tinderboxpoller set their polling interval
to taste rather than be overly aggressive on the poll.

I'm going to play with this a bit locally and see how it works. Review
pending...

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 269210 [details] [diff] [review]
update to tip, timeout

This works nicely. Also, thanks for including a unit test for it. Good stuff!

Comment 7

[Axel Hecht [:Pike]]

Rob checked this in, marking FIXED. I'll file a follow-up bug on bonsaipoller, that would want the logical sugar, and the timeout for real.

Comment 8

[Rob Campbell [:rc] (:robcee)]

whups. Checkin log:

cvs commit: Examining .
Checking in tinderboxpoller.py;
/cvsroot/mozilla/tools/buildbot/buildbot/changes/tinderboxpoller.py,v  <--  tinderboxpoller.py
new revision: 1.3; previous revision: 1.2
done