From: daniel@...42.com Date: 2021-04-22T14:34:44+00:00 Subject: [ruby-core:103552] [Ruby master Feature#17795] Around `Process.fork` callbacks API Issue #17795 has been updated by Dan0042 (Daniel DeLorme). akr (Akira Tanaka) wrote in #note-17: > Another idea is introducing Process.fork_level which can be used to detect fork instead of getpid. If that's possible, then it should be equally possible to just [pre-]cache `Process.pid` and it would be just as performant, right? Whatever were the reasons for pid cache removal in glibc, they would also apply to `fork_level`. From what I understand above it's not relevant here because calling `fork(2)` would leave the VM in an unusable state. Since checking the pid is a pattern that already exists, imho it's better to make that pattern performant than to introduce a new pattern with almost no advantage. I don't think pid reuse/collision is a realistic concern either. But checking either `Process.pid` or `fork_level` doesn't fix one of the biggest issues I've had with forking: _finalizers_. If the database object has a finalizer that closes the connection, you need to prevent that finalizer from ever executing in the child process. You can use `fork{ work(); exit! }` to prevent finalizers from running on exit, but that also prevents any finalizers that were defined in the child process. So you actually need to use `fork{ at_exit{exit!}; work() }`, but even then it doesn't cover the case where a finalizer is run when the object is garbage-collected. This is a real thorn, and to me it's the main reason why the socket must be closed in the child process right after fork. byroot (Jean Boussier) wrote in #note-23: > But I (and apparently a bunch of other library writers, since clearly a bunch of libraries already do it, and other languages offer that capability) disagree with this stance, and don't understand how Ruby design should be influenced by this types of "proper way to do it" arguments. +1; for me I believe fork safety should be handled by libraries because of encapsulation. If you have independant libs, libA that needs to close sockets on fork, and libB that uses fork, these two should remain independant. You should not need the app to insert glue code in order for the two libs to play nice together. That really breaks encapsulation and separation of concerns. 2� > Make Kernel.fork a delegator I have two concerns with this: 1. Some existing gems already override both `Kernel#fork` and `Process.fork` in order to setup callbacks; this change would cause the callbacks to be triggered twice when using `Kernel#fork`. So it's somewhat backward-incompatible. 2. You need to handle the with-block and without-block forms differently. So I'd like to make a counter-proposal: what about introducing a separate method (let's say `Process._fork_`) which _doesn't accept a block_ and is called by the various forking methods, including `IO.popen("-")`. That would solve the two concerns above, and it might even be simple enough to be backported. ---------------------------------------- Feature #17795: Around `Process.fork` callbacks API https://bugs.ruby-lang.org/issues/17795#change-91653 * Author: byroot (Jean Boussier) * Status: Open * Priority: Normal ---------------------------------------- Replaces: https://bugs.ruby-lang.org/issues/5446 ### Context Ruby code in production is very often running in a forking setup (puma, unicorn, etc), and it is common some types of libraries to need to know when the Ruby process was forked. For instance: - Most database clients, ORMs or other libraries keeping a connection pool might need to close connections before the fork happens. - Libraries relying on some kind of dispatcher thread might need to restart the thread in the forked children, and clear any internal buffer (e.g. statsd clients, newrelic_rpm). **This need is only for forking the whole ruby process, extensions doing a `fork(2) + exec(2)` combo etc are not a concern, this aim at only catching `kernel.fork`, `Process.fork` and maybe `Process.daemon`.**. The use case is for forks that end up executing Ruby code. ### Current solutions Right now this use case is handled in several ways. #### Rely on the integrating code to call a `before_fork` or `after_fork` callback. Some libraries simply rely on documentation and require the user to use the hooks provided by their forking server. Examples: - Sequel: http://sequel.jeremyevans.net/rdoc/files/doc/fork_safety_rdoc.html - Rails's Active Record: https://devcenter.heroku.com/articles/concurrency-and-database-connections#multi-process-servers - ScoutAPM (it tries to detect popular forking setup and register itself): https://github.com/scoutapp/scout_apm_ruby/blob/fa83793b9e8d2f9a32c920f59b57d7f198f466b8/lib/scout_apm/environment.rb#L142-L146 - NewRelic RPM (similarly tries to register to popular forking setups): https://www.rubydoc.info/github/newrelic/rpm/NewRelic%2FAgent:after_fork #### Continuously check `Process.pid` Some libraries chose to instead keep the process PID in a variable, and to regularly compare it to `Process.pid` to detect forked children. Unfortunately `Process.pid` is relatively slow on Linux, and these checks tend to be in tight loops, so it's not uncommon when using these libraries to spend `1` or `2%` of runtime in `Process.pid`. Examples: - Rails's Active Record used to check `Process.pid` https://github.com/Shopify/rails/blob/411ccbdab2608c62aabdb320d52cb02d446bb39c/activerecord/lib/active_record/connection_adapters/abstract/connection_pool.rb#L946, it still does but a bit less: https://github.com/rails/rails/pull/41850 - the `Typhoeus` HTTP client: https://github.com/typhoeus/typhoeus/blob/a345545e5e4ac0522b883fe0cf19e5e2e807b4b0/lib/typhoeus/pool.rb#L34-L42 - Redis client: https://github.com/redis/redis-rb/blob/6542934f01b9c390ee450bd372209a04bc3a239b/lib/redis/client.rb#L384 - Some older versions of NewRelic RPM: https://github.com/opendna/scoreranking-api/blob/8fba96d23b4d3e6b64f625079c184f3a292bbc12/vendor/gems/ruby/1.9.1/gems/newrelic_rpm-3.7.3.204/lib/new_relic/agent/harvester.rb#L39-L41 #### Continuously check `Thread#alive?` Similar to checking `Process.pid`, but for the background thread use case. `Thread#alive?` is regularly checked, and if the thread is dead, it is assumed that the process was forked. It's much less costly than a `Process.pid`, but also a bit less reliable as the thread could have died for other reasons. It also delays re-creating the thread to the next check rather than immediately upon forking. Examples: - `statsd-instrument`: https://github.com/Shopify/statsd-instrument/blob/0445cca46e29aa48e9f1efec7c72352aff7ec931/lib/statsd/instrument/batched_udp_sink.rb#L63 #### Decorate `Kernel.fork` and `Process.fork` Another solution is to prepend a module in `Process` and `Kernel`, to decorate the fork method and implement your own callback. It works well, but is made difficult by `Kernel.fork`. Examples: - Active Support: https://github.com/rails/rails/blob/9aed3dcdfea6b64c18035f8e2622c474ba499846/activesupport/lib/active_support/fork_tracker.rb - `dd-trace-rb`: https://github.com/DataDog/dd-trace-rb/blob/793946146b4709289cfd459f3b68e8227a9f5fa7/lib/ddtrace/profiling/ext/forking.rb - To some extent, `nakayoshi_fork` decorates the `fork` method: https://github.com/ko1/nakayoshi_fork/blob/19ef5efc51e0ae51d7f5f37a0b785309bf16e97f/lib/nakayoshi_fork.rb ### Proposals I see two possible features to improve this situation: #### Fork callbacks One solution would be for Ruby to expose a callback API for these two events, similar to `Kernel.at_exit`. Most implementations of this functionnality in other languages ([C's `pthread_atfork`](https://man7.org/linux/man-pages/man3/pthread_atfork.3.html), [Python's `os.register_at_fork`](https://docs.python.org/3/library/os.html#os.register_at_fork)) expose 3 callbacks: - `prepare` or `before` executed in the parent process before the `fork(2)` - `parent` or `after_in_parent` executed in the parent process after the `fork(2)` - `child` or `after_in_child` executed in the child process after the `fork(2)` A direct translation of such API in Ruby could look like `Process.at_fork(prepare: Proc, parent: Proc, child: Proc)` if inspired by `pthread_atfork`. Or alternatively each callback could be exposed idependently: `Process.before_fork {}`, `Process.after_fork_parent {}`, `Process.after_fork_child {}`. Also note that similar APIs don't expose any way to unregister callbacks, and expect users to use weak references or to not hold onto objects that should be garbage collected. Pseudo code: ```ruby module Process @prepare = [] @parent = [] @child = [] def self.at_fork(prepare: nil, parent: nil, child: nil) @prepare.unshift(prepare) if prepare # prepare callbacks are executed in reverse registration order @parent << parent if parent @child << child if child end def self.fork @prepare.each(&:call) if pid = Primitive.fork @parent.each(&:call) # We could consider passing the pid here. else @child.each(&:call) end end end ``` #### Make `Kernel.fork` a delegator A simpler change would be to just make `Kernel.fork` a delegator to `Process.fork`. This would make it much easier to prepend a module on `Process` for each library to implement its own callback. Proposed patch: https://github.com/ruby/ruby/pull/4361 -- https://bugs.ruby-lang.org/ Unsubscribe: