Skip to content

refactor(worker): Rework qworker teardown#299

Open
ketiltrout wants to merge 1 commit into
chime-upgradefrom
rework_qworker_shutdown
Open

refactor(worker): Rework qworker teardown#299
ketiltrout wants to merge 1 commit into
chime-upgradefrom
rework_qworker_shutdown

Conversation

@ketiltrout

@ketiltrout ketiltrout commented Jul 16, 2026

Copy link
Copy Markdown
Member

Fixes for both normal and abnormal termination of the qworker process:

  • the worker will now always print exceptions that kill it
  • the worker won't try to kill Sanic if it was Sanic that killed it.
  • SIGINT and SIGTERM properly handled
  • Redis connection cleanly closed before exit

Also:

  • Moved the giant go function out of the function it was embedded in for better code legibility. (There are no code changes in the go function.)

I think this may be at least partially responsible for some of the spurious test failures in the CI

Fixes for both normal and abnormal termination of the qworker process:
* the worker will now always print exceptions that kill it
* the worker _won't_ try to kill Sanic if it was Sanic that killed _it_.
* SIGINT and SIGTERM properly handled
* Redis connection cleanly closed before exit

Also:
* Moved the giant `go` function out of the function it was embedded in
  for better code legibility.

@tristpinsm tristpinsm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did my best to work through the exit paths and I think it all makes sense!

Comment thread coco/worker.py

# Normal exceptions should be supressed, BaseExceptions (e.g.
# KeyboardInterrupt) should be re-raised
if not isinstance(e, Exception):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not hurting anything, but I think this block is unreachable. If e was a BaseException, it would never make it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants