-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Register logfile before starting assignment #263
Conversation
self._started_deferred = None | ||
|
||
@property | ||
def stopped_deferred(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you go over the specifics as for why we're getting rid of these properties instead of updating them somehow? These are setup to try and ensure it's harder to missorder the setup of start_deferred/stop_deferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since Process._start() will now register the tasklog before actually starting the process, it will have to return these two properties before the process has actually started. So, yeah, in this case we will have to access both of these before self.start_called is set to True.
As for the problems described in the opening comments, I think I've gotten to the bottom of those, as well as, probably, the issues in #249. This seems to be a bug in Twisted-web (which treq uses as its backend), where if you do two consecutive, non-overlapping http requests with data against a server that doesn't support keep-alive, but doesn't make that explicit by setting a See: https://twistedmatrix.com/trac/ticket/7843 Since, in production, farms will probably use a real webserver instead of flask's built-in standalone mode, this doesn't seem like a major issue. Besides, as long as we're restarting the request when this happens, we're fine anyway. |
Interestingly the problem you're describing here is something I was able to replicate at work. I basically came to the same conclusions you did by accident when I did data = treq.json_content(response) when I meant to do data = yield treq.json_content(response) which in my case caused overlapping requests and it seemed to trigger the ConnectionDone error. I wanted to reply to your thread on twisted-web but was not subscribed at the time unfortunately. Regardless, nice work and thanks for taking a deep look at that. Anyway I'm going to go ahed and merge this, I think I have all my questions answered and I don't see anything code wise to fix here right now. |
Register logfile before starting assignment
Merging pyfarm#263 ended up breaking handling exceptions the jobtype's start() method. This should fix it again.
If we set the state of tasks not just on the task itself, but also on the current tasklog, we must make sure the tasklog is registered before we do that, otherwise that will fail with a 404.
In extreme cases, where assignments finish very fast, even uploading the final tasklog file at the end may fail because of this. (This can happen in practice if a jobtype class decides that no process needs to be started.)
Unfortunately, this PR still has a problem: When setting our agent's state, the request fails with this message:
It works on retry, but I have no idea where this problem comes from.