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 support for worker pre_shutdown_timeout #243

Closed

Conversation

amenon
Copy link

@amenon amenon commented May 12, 2024

Resque has had support for a pre_shutdown_timeout for a worker since v1.27.3 (resque/resque#1514).

The time between the parent receiving a shutdown signal (TERM by default) and it sending that signal on to the child process. Designed to give the child process time to complete before being forced to die.

This PR adds support for this attribute in the forked workers.

@amenon amenon changed the title Add support worker pre_shutdown_timeout Add support for worker pre_shutdown_timeout May 13, 2024
@nevans
Copy link
Collaborator

nevans commented May 13, 2024

I wonder if this is needed, and if so, why? Doesn't Resque::Worker#initialize already set it for itself:

    def initialize(*queues)
      @shutdown = nil
      @paused = nil
      @before_first_fork_hook_ran = false

      @heartbeat_thread = nil
      @heartbeat_thread_signal = nil

      @last_state = :idle

      verbose_value = ENV['LOGGING'] || ENV['VERBOSE']
      self.verbose = verbose_value if verbose_value
      self.very_verbose = ENV['VVERBOSE'] if ENV['VVERBOSE']
      self.pre_shutdown_timeout = (ENV['RESQUE_PRE_SHUTDOWN_TIMEOUT'] || 0.0).to_f
      self.term_timeout = (ENV['RESQUE_TERM_TIMEOUT'] || 4.0).to_f
      self.term_child = ENV['TERM_CHILD']
      self.graceful_term = ENV['GRACEFUL_TERM']
      self.run_at_exit_hooks = ENV['RUN_AT_EXIT_HOOKS']

      self.queues = queues
    end

Of course, the same question could be asked of the other config attributes set from environment variables in that method! So, I just looked at some versions of that method from earlier git tags, and it looks like it was updated to initialize itself from environment variables with version 1.26.0 (see also: version 1.25.2).

So, if this config setting was added by v1.27.3, I'm thinking that maybe we don't need to add it here? If the environment variable isn't updating the configuration (or achieving the desired results) maybe something else is wrong?

For what it's worth, the next time we make significant changes to this gem, its dependencies will probably be updated to require either resque >= 1.27. or 2.0.

Copy link
Collaborator

@nevans nevans left a comment

Choose a reason for hiding this comment

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

If you can show that this is needed, I'll merge it... I don't think it is though. The version of resque that has that config attribute will also set the config from the environment variable itself.

@amenon
Copy link
Author

amenon commented May 13, 2024

Thank you for the quick review @nevans. You are right that resque will load that config from the system environment variable. My initial test was this was flawed, but I have now verified that this works correctly. Sorry for wasting your time.

@amenon amenon closed this May 13, 2024
@nevans
Copy link
Collaborator

nevans commented May 14, 2024

@amenon no prob. I'm glad it worked out for you.

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.

2 participants