0.8.2: fix polling #18

Merged
merged 2 commits into from Oct 24, 2016

Projects

None yet

3 participants

@escapewindow
Member
  • The symptoms involved the polling dying, but not the git pull. Instead of relying on a single create_task and then an loop.run_forever() to poll, put a loop.run_until_complete() inside a while True loop. The git pulls are happening every 5 minutes as desired, and the taskcluster polling is still going.
  • Log the traceback if we hit any exceptions running async_main. I noticed that supervisor was restarting the process a lot while github was down on Friday, and I wasn't sure what was breaking. This should give us more info.
  • Remove the rebuild_gpg_homedirs_loop() basedir after we're done copying the contents, instead of at the beginning of the call. This helps avoid having background processes step on each other. We may end up having to monitor the tmpdir for freshness if the git pull hangs, but that hasn't been an issue so far.

Once I get review, I'm going to bump to 0.8.2 and merge, then cherry pick the fixes onto the 0.7.x branch for 0.7.2.

@JohanLorenzo , until @lundjordan gets back you're the one with the most scriptworker background; are you up for the review?

@coveralls

Coverage Status

Coverage remained the same at 100.0% when pulling 6096ed8 on escapewindow:fix_polling into b7a5a99 on mozilla-releng:master.

@JohanLorenzo

Looks good to me. I added some questions about the details of asyncio.

@@ -107,9 +107,17 @@ def main():
with aiohttp.ClientSession(connector=conn) as session:
context.session = session
context.credentials = credentials
+ tmp_gpg_home, _ = _get_tmp_gpg_home(context)
+ loop.create_task(
@JohanLorenzo
JohanLorenzo Oct 24, 2016 Contributor

ayncio noob question: What happens if we don't wait on the task created here? Does it still execute it somehow, but we don't have control on when it happens?

@escapewindow
escapewindow Oct 24, 2016 Member

A test script showed that when the loop continues, the background task happens during the awaits of the main task.

scriptworker/worker.py
+ loop.run_until_complete(async_main(context))
+ except Exception as exc:
+ log.warning(traceback.format_exc())
+ if not isinstance(exc, RuntimeError):
@JohanLorenzo
JohanLorenzo Oct 24, 2016 Contributor

If I understand correctly, this condition is here to filter out async errors like: RuntimeError: Event loop stopped before Future completed, isn't it?

If that's the case, I wonder if it wouldn't worth commenting it it the code, for future asyncio-rookies.

@escapewindow
escapewindow Oct 24, 2016 Member

Heh, I've been trying to figure why I put that in. =
Blame doesn't really tell me why: 8eca14e

The tests pass just fine without this (other than the one testing that we catch RuntimeError, of course). Either we leave this in, possibly with a comment that guesses as to the reason, or I take it out. I'm leaning towards the latter, since we'll log the error if we hit it.

CHANGELOG.md
@@ -3,6 +3,8 @@ All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).
## Unreleased
+### Fixed
@JohanLorenzo
JohanLorenzo Oct 24, 2016 Contributor

Maybe we could also append the part about logging every stacktrace for every kind of unhandled exceptions?

escapewindow added some commits Oct 20, 2016
@escapewindow escapewindow Fix task polling loop, and log traceback.
Stop catching `RuntimeError` since we're not sure why that was put in.
d1ca621
@escapewindow escapewindow 0.8.2
c261f91
@coveralls

Coverage Status

Coverage remained the same at 100.0% when pulling c261f91 on escapewindow:fix_polling into e4ced86 on mozilla-releng:master.

@escapewindow escapewindow merged commit 97dcafb into mozilla-releng:master Oct 24, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@escapewindow escapewindow deleted the escapewindow:fix_polling branch Jan 23, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment