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

Add SIGHUP because golang (and therefore docker) can't forward SIGCONT #1409

Closed
wants to merge 1 commit into from
Closed

Add SIGHUP because golang (and therefore docker) can't forward SIGCONT #1409

wants to merge 1 commit into from

Conversation

fw42
Copy link
Member

@fw42 fw42 commented Feb 8, 2016

This is a (rebased) cherry-pick of https://github.com/Shopify/resque/commit/96f4ddb80f5842d6f43323f0076a2f9cdcb22b33 and a work-around for golang/go#8953 (https://go-review.googlesource.com/#/c/18185/).

This PR effectively makes SIGHUP an alias of SIGCONT, so that we can actually use this functionality with Docker. This can be removed at some point in the future (who knows when), but since the upstream golang change is pretty recent, I think it might be worth it to merge it for now.

Ping @burke @steveklabnik @dylanahsmith for review.

Probably should ping someone (?) from Docker in case anyone cares or has similar problems with other applications that also want to use SIGCONT? cc @jfrazelle

@steveklabnik
Copy link
Member

Tweeted for some exposure, dunno how much attention that will get.

@ecoffey
Copy link
Member

ecoffey commented Feb 8, 2016

The patch looks clean and straightforward.

The only nervous part is you effectively bake into the public API of a resque worker a go/docker workaround.

I think the important question to answer before hitting "merge" is do we ever want to use SIGHUP for something else?

@calavera
Copy link

calavera commented Feb 8, 2016

Docker maintainer here

This looks like a good temporary patch. Go 1.6 will include the mentioned fix in the language, but we won't use it in a release until our next major one, v1.11, probably about April.

I agree with @ecoffey though.

@dylanahsmith
Copy link
Contributor

I think we should just recommend a workaround. I think the simplest workaround is to setup a signal handler to forward the signal, which could be done in the resque:setup rake task:

task "resque:setup" => :environment do
  trap('HUP') { Process.kill('CONT', Process.pid) } # remove once go and docker supports SIGCONT
end

@fw42
Copy link
Member Author

fw42 commented Feb 9, 2016

@dylanahsmith: Good idea. I like that much better. Closing here.

@fw42 fw42 closed this Feb 9, 2016
@fw42 fw42 deleted the upstream-sighup-shit branch February 9, 2016 01:00
@steveklabnik
Copy link
Member

👍

Maybe this should go in the README for the next few months?

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.

None yet

6 participants