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

feature request: make after_worker_fork to be called after forking mold #94

Closed
kyontan opened this issue Mar 5, 2024 · 5 comments
Closed

Comments

@kyontan
Copy link

kyontan commented Mar 5, 2024

Hello,

I'm trying to introduce Pitchfork to our application, and we are now facing the issues around use with gRPC gem.

gRPC gem provides three methods for forking the process:

  • GRPC.prefork
  • GRPC.postfork_child
  • GRPC.postfork_parent

The difficult point is that the postfork_child and postfork_parent should be called from respective processes after forking.
https://github.com/grpc/grpc/blob/db9ca10b351da70cd34359f8ea5ea00cbb4ffc1a/src/ruby/ext/grpc/rb_grpc.c#L363-L444

If I'm not mistaken, no hook is called after forking the mold process in the worker context.
So the my understanding is: I need to call GRPC.postfork_parent outside of the pitchfork's hook configuration such as before block in application.

Can we make Pitchfork to call after_worker_fork after forking the mold, or have another hook?

Ref:

Thanks,

@casperisfine
Copy link
Contributor

The difficult point is that the postfork_child and postfork_parent should be called from respective processes after forking.

Yeah, that's what Google devs told us, but we're not doing that.

Restarting GRPC in the mold, would mean you'd need to shut it down before you fork workers, so the mold would get much much slower.

We keep GRPC disabled in the mold and we're good with that. If you want to really restart it after every fork, decorating Process._fork is a better avenue IMO.

@kyontan
Copy link
Author

kyontan commented Mar 5, 2024

Restarting GRPC in the mold, would mean you'd need to shut it down before you fork workers, so the mold would get much much slower.
I see.

Currently, we are not using GRPC directly, and it's used in google-cloud-pubsub gem internally, and don't use GRPC gem directly for servers.

I understand keeping GRPC disabled in the mold is good to do, so we may avoid calling GRPC.enable in after_mold_fork, though we need to call GRPC.prefork in before_fork hook because GRPC may have used in the fork source process.
Then I still need the hook that will be called after forking mold in the worker context, or should I use hacky Process._fork instead?

@casperisfine
Copy link
Contributor

Yes, the API provided by GRPC is quite hard to work with. It will raise if you try to disable when it's already disabled etc.

Here's some code we use to handle this:

# frozen_string_literal: true

if defined? GRPC::Core # ::GRPC may be loaded by bundler from `grpc.gemspec` so we check `GRPC::Core`
  raise <<~ERROR
    GRPC was loaded before #{__FILE__} preventing to enable fork support
  ERROR
end

ENV['GRPC_ENABLE_FORK_SUPPORT'] = '1'
require 'grpc'

module GRPCClient
  GRPC_FORK_SUPPORT = GRPC.respond_to?(:prefork) && Process.respond_to?(:_fork) && RUBY_PLATFORM.match?(/linux/i)
  @disabled_by = nil

  class << self
    def enabled?
      @disabled_by.nil?
    end

    def disable
      return if @disabled_by

      GRPC.prefork if GRPC_FORK_SUPPORT
      @disabled_by = Process.pid

      nil
    end

    def resume
      return unless @disabled_by

      if @disabled_by == Process.pid
        GRPC.postfork_parent if GRPC_FORK_SUPPORT
      else
        GRPC.postfork_child if GRPC_FORK_SUPPORT
      end

      @disabled_by = nil
    end
  end

  if GRPC_FORK_SUPPORT
    module ForkSafety
      def _fork
        # If GRPC was already disabled, we leave it disabled
        return super unless GRPCClient.enabled?

        GRPCClient.disable

        pid = super

        if pid > 0 # parent
          # We always resume GRPC if we had to disable it to fork a child
          GRPCClient.resume
        end

        pid
      end

      # On Process.spawn actually does fork+exec when running as root
      # See: https://github.com/ruby/ruby/blob/fdb8f086396e0f9b64e069852cb0dd40147877d1/process.c#L3956-L3966
      if Process.uid.zero? # root
        def spawn(...)
          # If GRPC was already disabled, we leave it disabled
          return super unless GRPCClient.enabled?

          GRPCClient.disable

          pid = super

          GRPCClient.resume

          pid
        end
      end
    end

    def self.fork_safe!
      ::Process.singleton_class.prepend(ForkSafety)
    end
  else
    def self.fork_safe!
      # NOOP
    end
  end
end

GRPCClient.fork_safe!

This automatically disable GRPC before all forks, not just Pichfork reforking. The childs have GRPC disabled, and we can explictly re-enable it in after_worker_fork with GRPCClient.resume.

Hope this helps.

@casperisfine
Copy link
Contributor

Note to self: I should package that code in a small grpc-fork-safety gem or something.

@kyontan
Copy link
Author

kyontan commented Mar 5, 2024

Thanks for giving a very detailed snippets, it's long as expected 😇

Anyway, now I understand how we can make it forward, and thanks for your great help!

@kyontan kyontan closed this as completed Mar 5, 2024
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

No branches or pull requests

2 participants