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

Updater: Log info message when receiving signal #951

Merged
merged 5 commits into from
Jan 9, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AUTHORS.rst
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ The following wonderful people contributed directly or indirectly to this projec
- `Li-aung Yip <https://github.com/LiaungYip>`_
- `macrojames <https://github.com/macrojames>`_
- `Michael Elovskikh <https://github.com/wronglink>`_
- `Mischa Krüger <https://github.com/Makman2>`_
- `naveenvhegde <https://github.com/naveenvhegde>`_
- `neurrone <https://github.com/neurrone>`_
- `njittam <https://github.com/njittam>`_
Expand Down
1 change: 1 addition & 0 deletions requirements-dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ yapf
pre-commit
beautifulsoup4
pytest
pytest-capturelog
pytest-timeout
3 changes: 3 additions & 0 deletions telegram/ext/updater.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
from telegram import Bot, TelegramError
from telegram.ext import Dispatcher, JobQueue
from telegram.error import Unauthorized, InvalidToken, RetryAfter
from telegram.utils.helpers import get_signal_name
from telegram.utils.request import Request
from telegram.utils.webhookhandler import (WebhookServer, WebhookHandler)

Expand Down Expand Up @@ -449,6 +450,8 @@ def _join_threads(self):
def signal_handler(self, signum, frame):
self.is_idle = False
if self.running:
self.logger.info('Received signal {} ({}), stopping...'.format(
signum, get_signal_name(signum)))
self.stop()
if self.user_sig_handler:
self.user_sig_handler(signum, frame)
Expand Down
13 changes: 13 additions & 0 deletions telegram/utils/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,26 @@
"""This module contains helper functions."""

import re
import signal
from datetime import datetime

try:
from html import escape as escape_html # noqa: F401
except ImportError:
from cgi import escape as escape_html # noqa: F401


# From https://stackoverflow.com/questions/2549939/get-signal-names-from-numbers-in-python
_signames = {v: k
for k, v in reversed(sorted(vars(signal).items()))
Copy link
Member

Choose a reason for hiding this comment

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

nitpicking here. the reversed & sorted are useless here as you're creating a dict. please remove them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the sorted was there for signals with different names but same number, so there's a deterministic outcome. However I haven't understood the reversed call too^^

Copy link
Contributor Author

@Makman2 Makman2 Dec 30, 2017

Choose a reason for hiding this comment

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

And kk reversed then picks signal names that are sorted at the beginning. On my system I get these differences when I don't use reversed:

SIGIOT vs SIGABRT
SIGCLD vs SIGCHLD
SIGPOLL vs SIGIO

Though not sure if it's that important here^^

Copy link
Member

Choose a reason for hiding this comment

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

Check manpage of signal(7) all the above are synonyms. If one name is chosen over another it's merely a matter of luck, as dictionary order is not guaranteed.
On my system I had different results on different executions.

Another option is to leave the signal name alone and just print the number.

Copy link
Contributor Author

@Makman2 Makman2 Jan 7, 2018

Choose a reason for hiding this comment

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

If one name is chosen over another it's merely a matter of luck, as dictionary order is not guaranteed.

Looks like it's true, I'm just wondering: the order we iterate over should always produce a guaranteed outcome. vars(signal).items() has unique mappings from name -> number. Using sorted we get always the same list with the same order, and then we are filling the dict with those values, while switching name and number. If there are numbers that are equal, later elements in the list should overwrite the old ones. Although using dict-comprehension, citing the dict-docs:

If a key occurs more than once, the last value for that key becomes the corresponding value in the new dictionary.

Copy link
Contributor Author

@Makman2 Makman2 Jan 7, 2018

Choose a reason for hiding this comment

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

Another option is to leave the signal name alone and just print the number.

I wouldn't do that, signal numbers differ on OSes for the "same" signals. I think people identify those signals usually by their name, like SIGINT / SIGTERM.

Copy link
Member

Choose a reason for hiding this comment

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

ok. not worth the argument, lets keep it as is.

if k.startswith('SIG') and not k.startswith('SIG_')}


def get_signal_name(signum):
"""Returns the signal name of the given signal number."""
return _signames[signum]


# Not using future.backports.datetime here as datetime value might be an input from the user,
# making every isinstace() call more delicate. So we just use our own compat layer.
if hasattr(datetime, 'timestamp'):
Expand Down
22 changes: 17 additions & 5 deletions tests/test_updater.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
#
# You should have received a copy of the GNU Lesser Public License
# along with this program. If not, see [http://www.gnu.org/licenses/].
import logging
import os
import signal
import sys
from functools import partial
from queue import Queue
from random import randrange
from threading import Thread
Expand Down Expand Up @@ -238,15 +240,25 @@ def _send_webhook_msg(self,

return urlopen(req)

def signal_sender(self):
def signal_sender(self, updater):
sleep(0.2)
while not updater.running:
sleep(0.2)

os.kill(os.getpid(), signal.SIGTERM)

@signalskip
def test_idle(self, updater):
def test_idle(self, updater, caplog):
updater.start_polling(0.01)
Thread(target=self.signal_sender).start()
updater.idle()
Thread(target=partial(self.signal_sender, updater=updater)).start()

with caplog.atLevel(logging.INFO):
updater.idle()

rec = caplog.records()[-1]
assert rec.msg.startswith('Received signal {}'.format(signal.SIGTERM))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just curious, why not asserting for the complete message? :)

assert rec.levelname == 'INFO'

# If we get this far, idle() ran through
sleep(.5)
assert updater.running is False
Expand All @@ -260,7 +272,7 @@ def user_signal_inc(signum, frame):

updater.user_sig_handler = user_signal_inc
updater.start_polling(0.01)
Thread(target=self.signal_sender).start()
Thread(target=partial(self.signal_sender, updater=updater)).start()
updater.idle()
# If we get this far, idle() ran through
sleep(.5)
Expand Down