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

allow app to also handle sighup and call super #1527

Closed
wants to merge 1 commit into from

Conversation

grosser
Copy link
Contributor

@grosser grosser commented Mar 9, 2018

our app wants to reopen rails logs on sighup
to do that it needs to trap sighup too
but calling pumas trap after does not work since it crashes the server if redirect_io was not used

with this change there is 1 less trap used and our app can only call pumas trap if it was set

see zendesk/samson#2637

/cc @ragurney @jonmoter

our app wants to reopen rails logs on sighup
to do that it needs to trap sighup too
but calling pumas trap after does not work since it crashes the server if redirect_io was not used

with this change there is 1 less trap used and our app can only call pumas trap if it was set
@nateberkopec
Copy link
Member

Doesn't this change mean SIGHUP will no longer call stop?

@grosser
Copy link
Contributor Author

grosser commented Mar 20, 2018

it will kill the process (default behavior of SIGHUP) ... not sure if that's bad ... would certainly stop the server :D

ruby -e sleep
kill -HUP 39052
Hangup: 1

@runner.redirect_io
else
Copy link
Member

Choose a reason for hiding this comment

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

@grosser What I meant was you eliminated this branch of code, and I don't understand why.

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 was basically thinking "if you send SIGHUP to a server that does not have any reload mechanic then wtf are you doing" ... but maybe it's valid usecase :(

Copy link
Member

Choose a reason for hiding this comment

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

I think it was important to #1377?

@grosser
Copy link
Contributor Author

grosser commented Mar 27, 2018 via email

@nateberkopec
Copy link
Member

SIGWHYDONTWECALLTHEWHOLETHINGOFF

@grosser
Copy link
Contributor Author

grosser commented Mar 28, 2018

#911 has reproduction steps, so if they still work then it's ok ?
it's not really an urgent/important change, just 1-more-weirdness in the codebase 🤷‍♂️

@evanphx
Copy link
Member

evanphx commented Feb 20, 2019

I'm going to reject this PR because it changes the meaning of SIGHUP for existing users.

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

Successfully merging this pull request may close these issues.

None yet

3 participants