Fix SignalConnector._has_already_handler check for callable type

[mauvilsa]

What does this PR do?

Fixes an issue where a user's registered SIGTERM signal handler is overwritten by SignalConnector.register_signal_handlers. One case in which this happens is when a method of a class instance is registered. That is:

import signal

class Callback:
    def __init__(self):
        signal.signal(signal.SIGTERM, self.sigterm_handler)
    def sigterm_handler(self):
        pass

callback = Callback()

The current check isinstance(signal.getsignal(signal.SIGTERM), FunctionType) evaluates to false, thus overwriting it.

No related issue. This was discussed over slack.

Does your PR introduce any breaking changes? If yes, please list them.

Before submitting

• Was this discussed/approved via a GitHub issue? (not for typos and docs)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together?
• Did you make sure to update the documentation with your changes? (if necessary)
• Did you write any new necessary tests? (not for typos and docs)
• Did you verify new and existing tests pass locally with your changes?
• Did you list all the breaking changes introduced by this pull request?
• Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

• Is this pull request ready for review? (if not, please submit in draft mode)
• Check that all items from Before submitting are resolved
• Make sure the title is self-explanatory and the description concisely explains the PR
• Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

[carmocca]

LGTM. Can you add a test to avoid regressions? Can be a simple unit-test. _has_already_handler can be made static for it.

[mauvilsa]

Can you add a test to avoid regressions? Can be a simple unit-test.

Done.

[tchaton]

LGTM !

[awaelchli]

Interesting, wasn't aware of this PR, but I have also one here #10611 where I changed the has_already_sampler to return

return signal.getsignal(signum) is not signal.SIG_DFL

Is the check for callable necessary? Which way is the correct way to check this, anyone knows? :)

[mauvilsa]

Is the check for callable necessary? Which way is the correct way to check this, anyone knows? :)

I guess the callable is not necessary. What should be the behavior if the value is signal.SIG_IGN which is part of the same enum that defines signal.SIG_DFL.

[awaelchli]

Good point, I think we should respect it and not add a handler in that case too?

[codecov]

Codecov Report

Merging #10483 (6bc06ab) into master (32e6522) will increase coverage by 45%.
The diff coverage is 100%.

❗ Current head 6bc06ab differs from pull request most recent head 6ed1213. Consider uploading reports for the commit 6ed1213 to get more accurate results

@@            Coverage Diff            @@
##           master   #10483     +/-   ##
=========================================
+ Coverage      43%      88%    +45%     
=========================================
  Files         177      177             
  Lines       16520    16506     -14     
=========================================
+ Hits         7034    14534   +7500     
+ Misses       9486     1972   -7514