From: merch-redmine@... Date: 2021-05-21T21:23:48+00:00 Subject: [ruby-core:103959] [Ruby master Feature#17795] Around `Process.fork` callbacks API Issue #17795 has been updated by jeremyevans0 (Jeremy Evans). I still believe neither of these proposals (fork callbacks or Kernel.fork delegation) should be accepted. In general, the proposals are for the same feature (wrapping behavior on fork), so in my opinion there is not much difference between them, and this response will treat them as being the same. Here's what I consider the primary benefit of adding support for fork callbacks: * They handle the common case easily with no code required by the library user. Here's what I consider the primary cost of adding support for fork callback: * They invoke the fork callbacks when they should not be invoked, because libraries cannot know the purpose of the fork in other libraries or in application code. I don't want to understate the benefit, language-level fork callbacks make certain common cases easier, and that is a valuable goal. However, I consider the cost too high. Without reiterating my previous points too much, not all forks are the same. A fork made by a webserver library is different than a fork made in application code. With language-level fork callbacks, the callback has no context about why the fork is happening, and therefore cannot make appropriate decisions. With the current situation, each library that uses fork generally implements their own callbacks around the library's use of fork, and users use those library-specific callbacks as appropriate to handle resources (e.g. closing sockets). This results in no problems when library-specific callbacks are used correctly. Almost all of the problems in this area occur when the library-specific callbacks should be used and are not. What will likely happen if this is implemented is that libraries will no longer implement fork callbacks, instead telling users to use language-level fork callbacks. Worse, libraries that create resources (e.g. opening sockets) will start to automatically use the language-level fork callbacks to close the resources on fork, regardless of whether doing so makes sense for the application using the library. This will lead to a situation where the common case is easier but the more complex case is broken completely with no ability to be fixed. If we are lucky, the libraries that create resources will offer a way to opt out of using the callbacks automatically. All of this is speculation on my part, but that is the future I foresee if this proposal is accepted. In terms of Sequel, since it has been mentioned, only disconnecting before fork is considered safe. Disconnecting after fork is not safe as forks could be operating on the same connection, and Sequel may not have direct access to the underlying socket to close (Sequel supports 10+ adapters built-in, and more in external gems). Disconnecting before fork can only be done safely if you are sure nothing else is using the connections, such as before a web server forks child processes. At any other point, automatic disconnection before fork would be terrible, as the connections could be in use by other threads. Another, more minor issue, is that a language-level before fork callback will necessarily be called on every fork, whereas current library-specific before fork callbacks are generally only called once before forking multiple times. I think the problem does not have a good solution. However, in my opinion, the current situation is the least worst. I don't think libraries should try to detect forks using ` Process.pid` or `Thread.alive?` or `fork_level`. Libraries should document how they should be used with fork, and should rely on the user correctly disconnecting before or after fork. Users that do not do so will likely have problems. To paraphrase Farquaad, "Some processes may die, but it's a sacrifice I am willing to make". :) ---------------------------------------- Feature #17795: Around `Process.fork` callbacks API https://bugs.ruby-lang.org/issues/17795#change-92091 * 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: