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

Don't nick SIGTERM #112

Closed
nilstoedtmann opened this issue Mar 16, 2017 · 7 comments
Closed

Don't nick SIGTERM #112

nilstoedtmann opened this issue Mar 16, 2017 · 7 comments
Assignees

Comments

@nilstoedtmann
Copy link
Contributor

nilstoedtmann commented Mar 16, 2017

bacpypes.core sets bacpypes.core.stop() as signal handler for SIGTERM. That stops any application that imports bacpypes from catching SIGTERM itself for a graceful shutdown.

We are happy to call bacpypes.core.stop() in our SIGTERM handler.

Or maybe make it configurable.

@JoelBender
Copy link
Owner

Humm... how about sigterm and sigusr1 as new keyword parameters to the run function with the existing functions as defaults?

def run(spin=SPIN, sigterm=stop, sigusr1=print_stack):
    ...
    if (sigterm is not None) and hasattr(signal, 'SIGTERM'):
        signal.signal(signal.SIGTERM, sigterm)
    if (sigusr1 is not None) and hasattr(signal, 'SIGUSR1'):
        signal.signal(SIGUSR1, sigusr1)
    ...

Your applications could pass in None. I'm not sure how many applications rely on the current behavior, apart from mine. I would like to hear from more users.

@nilstoedtmann
Copy link
Contributor Author

👍 That should work for us. Good not to set the handler at import time.

@JoelBender
Copy link
Owner

Ok, this is now sitting in the 112-dont-nick-sigterm branch.

@JoelBender JoelBender self-assigned this Mar 21, 2017
@nilstoedtmann
Copy link
Contributor Author

nilstoedtmann commented Mar 21, 2017

BTW you could also check whether the signal in question has already a handler assigned. See e.g. http://stackoverflow.com/a/12018294 .
I don't have a preference, just saying.

@nilstoedtmann
Copy link
Contributor Author

Either way I hope this will find its way into the next release :)

JoelBender added a commit that referenced this issue Mar 26, 2017
@JoelBender
Copy link
Owner

I missed this with 0.15.1 out of git branch sync confusion with the different platforms I'm using for testing, but it will be in 0.15.2 shortly.

@JoelBender
Copy link
Owner

This has introduced a bug, see #119 and continue the discussion...

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

No branches or pull requests

2 participants