Fix/reload jobs block event loop - #553
Conversation
Reloading the jobs dir froze the event loop for 6-9s, stalling the UI, scheduling, and websockets. The loader consumed `list_yaml_files` into a list up front, running the whole (slow, NFS) `os.walk` before any await; the `to_thread` parse didn't help since ruamel holds the GIL. Iterate the lazy generator and `await asyncio.sleep(0)` per file, so the walk/parse interleave with the loop. Parse inline now that to_thread is gone. Adds a test asserting a concurrent task keeps running during a slow reload.
Swap ruamel's YAML().load for DupCheckSafeLoader, a yaml.CSafeLoader (libyaml) subclass, and parse each file in asyncio.to_thread. Preserve ruamel's behavior: duplicate-key detection and YAML 1.2 scalar resolution, plus merge keys and empty-scalar-as-null. Malformed YAML is now collected as a per-job error instead of aborting the reload.
| # Load one file at a time, yielding to the loop after each, so a reload | ||
| # doesn't block the event loop. | ||
| for path, job_id in list_yaml_files(jobs_path): | ||
| _, job, exc = await load_job(path, job_id) |
There was a problem hiding this comment.
I've lost the description you gave me over slack that had benchmark numbers. Is this slower overall because you're parsing the yaml sequentially instead of in a threadpool?
Granted that performance doesn't matter too much here as long as we're not blocking the event loop, which we don't seem to be.
| """A YAML mapping contains a duplicate key.""" | ||
|
|
||
|
|
||
| class _DupCheckSafeLoader(yaml.CSafeLoader): |
There was a problem hiding this comment.
Remind me, what's the performance payoff of using this custom loader? Tweaking it to this extent makes me a bit anxious.
Remember, absolute job load performance isn't the top priority here. It's more not blocking the event loop and being 100% correct.
| dir_path = Path(dir_path) | ||
| for dir, _, names in os.walk(dir_path): | ||
| for dir, dirs, names in os.walk(dir_path): | ||
| # Don't go into hidden dirs (e.g. `.git`) |
| jobs[job_id] = job | ||
| if exc is not None: | ||
| errors.append(exc) | ||
| # Load one file at a time. `load_job` parses in a thread, which yields to the loop, so a reload doesn't block it. |
There was a problem hiding this comment.
You could tighten up this comment. It's not the number of jobs loaded at a time that was blocking the event loop. It was that there was a synchronous walk of the jobs directory, which this now does lazily.
2 participants
Opening for @fanman1's branch because it was blocked for him at work.