Skip to content
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

Fix request_force_stop_sigrtmin failing for python3 #727

Merged
merged 2 commits into from
Jul 24, 2016

Conversation

spiliopoulos
Copy link
Contributor

@spiliopoulos spiliopoulos commented Jul 24, 2016

The signal handler request_force_stop_sigrtmin (introduced with the heroku specific worker, #715) would fail in python3 because it was trying to read frame properties that have been removed in python3.
We now handle python2 as a special case with more frame properties.

Also fixed some warnings about using the war function of the logger

Yannis Spiliopoulos added 2 commits July 24, 2016 12:29
request_force_stop_sigrtmin would fail for python3 because it would
try to read frame attributes that have been removed in python3

This patch fixes that by reading more fram attributes only for
python2
@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage decreased (-0.04%) to 85.931% when pulling 7d0895e on spiliopoulos:fix_failing_heroku_signal_handler into 11f9833 on nvie:master.

@coveralls
Copy link

coveralls commented Jul 24, 2016

Coverage Status

Coverage increased (+0.02%) to 85.992% when pulling 7d0895e on spiliopoulos:fix_failing_heroku_signal_handler into 11f9833 on nvie:master.

@selwin selwin merged commit a0497a2 into rq:master Jul 24, 2016
@selwin
Copy link
Collaborator

selwin commented Jul 24, 2016

Thanks!

'f_lasti', 'f_lineno', 'f_locals', 'f_restricted', 'f_trace']

frame_properties = ['f_code', 'f_lasti', 'f_lineno', 'f_locals', 'f_trace']
if sys.version_info[:2] < (3, 0):
Copy link
Member

Choose a reason for hiding this comment

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

Would have been good to get a chance to review this, surely no need to merge it immediately?

Would it not have been better to use if PY2 from compat? That's surely what compat is there for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I completely forgot about compat. I'll fix that and work also on a test about it too as you suggested.

Copy link
Member

Choose a reason for hiding this comment

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

done, see #729

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry about that, was in a hurry. Thanks for the improvements.

@samuelcolvin
Copy link
Member

Also (and arguably the first step in fixing this) we should have a test that the error raised is indeed ShutdownImminentException otherwise this might happen again.

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.

4 participants