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

monitor children and re-fork when they exit #615

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dgobaud
Copy link

@dgobaud dgobaud commented Jan 9, 2014

Adding to wkonkel's pull request #565 - adding code to monitor children and re-fork them when they exit.

@albus522
Copy link
Member

From experience trying to do this, I can say there are a bunch of edge cases this does not cover. Just use the DJ binary.

@albus522 albus522 closed this Sep 19, 2014
@dgobaud
Copy link
Author

dgobaud commented Sep 19, 2014

what is the dj binary/does it cover the issue in #565 ?

@albus522
Copy link
Member

bin/delayed_job or script/delayed_job for older versions of rails

Sent from my iPhone

On Sep 19, 2014, at 5:42 PM, David Gobaud [email protected] wrote:

what is the dj binary/does it cover the issue in #565 ?


Reply to this email directly or view it on GitHub.

@dgobaud
Copy link
Author

dgobaud commented Sep 19, 2014

but those don't support NUM_PROCESSES for running multiple workers on a single dyno correct?

@albus522
Copy link
Member

You are correct -n is not supported in such a way that heroku can support it.

The problem is that while your code will possibly work for your use case, it will not work in general. If we included this in the release, we would receive a bunch of issue reports of this not working in all situations.

In order to be included in the main release, the code needs to work for most cases, not just one.

@dgobaud
Copy link
Author

dgobaud commented Sep 23, 2014

Got it - I understand it doesn't make sense to include if it causes problems. What are the problems/bugs? I'd like to fix them if possible.

@albus522
Copy link
Member

Lets clean this up and pull it in as a use at your own risk feature.

  • Maintain worker name. Store the pids and detect which worker died. wait will return the pid and exit status of the dead process.
  • Add logging for worker died
  • Cleanup forked workers on exit
  • Move the fork_delayed definition out into the namespace
  • while doesn't need do

@albus522 albus522 reopened this Sep 24, 2014
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.31%) when pulling a6c79c7 on dgobaud:master into 42b1489 on collectiveidea:master.

1. Maintain and log worker name and pid on fork
2. Detect and log which worker died.
3. Move the fork_delayed definition out into the namespace
4. Remove do from while loop
5. Trap TERM and stop creating new workers
@dgobaud
Copy link
Author

dgobaud commented Sep 25, 2014

Sounds good. I've made all the changes except "Cleanup forked workers on exit" - I'm not sure how best to do that.

@joshco
Copy link

joshco commented Oct 8, 2014

+! here for a solution to this for heroku. I tried a workaround using a shell script, but for some reason logging gets lost.
The logic is to start a bunch of daemons, then give the parent process something to do as in 'rake jobs:work' so you end up with some running as daemons and one running in the foreground to keep the dyno alive. However, the logging gets lost as STDOUT doesnt pass thru.

Here's my shell script (joshco_jobs)

#!/usr/bin/env bash

# dj spawner
bundle exec bin/delayed_job -n $DJ_DAEMONS start
bundle exec rake jobs:work

My procfile entry

jobber: bundle exec bin/joshco_jobs

@johnnyshields
Copy link

This PR can be closed in favor of #1138

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

5 participants