Skip to content

Since 1.12.1: Shutdown (or rather startup?) fails due to unreferenced variable #463

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

Closed
aberres opened this issue Jan 12, 2022 · 5 comments · Fixed by #464
Closed

Since 1.12.1: Shutdown (or rather startup?) fails due to unreferenced variable #463

aberres opened this issue Jan 12, 2022 · 5 comments · Fixed by #464

Comments

@aberres
Copy link
Contributor

aberres commented Jan 12, 2022

What version of Dramatiq are you using?

1.21.1

What did you do?

Since the update from 1.12.0 to 1.12.1 we are seeing the following errors:

 NameError: free variable 'running' referenced before assignment in enclosing scope
at .termhandler ( /venv/lib/python3.9/site-packages/dramatiq/cli.py:393 )
at .pthread_sigmask ( /usr/local/lib/python3.9/signal.py:60 )
at .try_unblock_signals ( /venv/lib/python3.9/site-packages/dramatiq/cli.py:238 )
at .worker_process ( /venv/lib/python3.9/site-packages/dramatiq/cli.py:409 )
at .run ( /usr/local/lib/python3.9/multiprocessing/process.py:108 )
at ._bootstrap ( /usr/local/lib/python3.9/multiprocessing/process.py:315 ) 

I could not yet investigate further, but maybe it is related to 8da5157?

@FinnLidbetter
Copy link
Contributor

Oh maybe just moving running = True above the try_unblock_signals will fix this

dramatiq/dramatiq/cli.py

Lines 408 to 412 in 7557943

# Unblock the blocked signals inherited from the parent process.
try_unblock_signals()
running = True
while running:

@FinnLidbetter
Copy link
Contributor

Or probably moving running = True to above the definition of term_handler makes more sense (e.g., to line 390)

@Bogdanp
Copy link
Owner

Bogdanp commented Jan 13, 2022

While @FinnLidbetter 's change seems appropriate, I wonder if you could give us more details about your setup @aberres. What OS are you on, and how are you stopping the process? Does it always happen? The window in which this can happen at the moment (given the current implementation) seems extremely narrow, and I'm unable to reproduce the issue.

@aberres
Copy link
Contributor Author

aberres commented Jan 13, 2022

Yes, sure.

The environment this happens on are Ubuntu Docker containers running on GKE (Google Kubernetes Engine). As we shut down/restart worker processes depending on available workload and additionally spin up complete environments for every branch in CI we got quite some shutdowns and restarts of workers. My gut feeling is that we are not seeing this issue most of the time.

I had another look at the logs and I'm a bit puzzled. It seems as if this message is not thrown during shutdown, but during startup.

If I read the logs correctly, sometimes SIGTERM is triggered pretty much instantly.

Sorry for the screenshots, but getting human readable text logs out of the Google Logs Explorer does not seem to be straight forward.

Logs

Note the second line, right after starting:

image

Some time later (we are waiting for a database instance to come up) the startup process continues as expected:

image

And then boom

image

@aberres aberres changed the title Since 1.12.1: Shutdown fails due to unreferenced variable Since 1.12.1: Shutdown (or rather startup?) fails due to unreferenced variable Jan 13, 2022
@Bogdanp
Copy link
Owner

Bogdanp commented Jan 13, 2022

If I read the logs correctly, sometimes SIGTERM is triggered pretty much instantly.

OK, that's what I suspected. In that case, Finn's change should fix this problem. Thanks for the details!

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 a pull request may close this issue.

3 participants