From: merch-redmine@... Date: 2021-04-13T02:32:07+00:00 Subject: [ruby-core:103411] [Ruby master Feature#17795] `before_fork` and `after_fork` callback API Issue #17795 has been updated by jeremyevans0 (Jeremy Evans). The main issue I see with this is the potential for misuse. If I could be sure this would only be used by applications and not libraries, it seems reasonable. However, I suspect the main usage will be libraries, and it's not really possible to use this correctly in libraries. For example, this currently works: ```ruby Sequel.postgres.transaction do exit! if fork end ``` So does this: ```ruby Sequel.postgres.transaction do exit! unless fork end ``` If a library starts using a `before_fork` or `after_fork` hook to close connections automatically, it breaks at least one of the two cases. Automatically disconnecting in `before_fork` will break both cases, since the connection is disconnected before the fork. Automatically disconnecting in `after_fork` breaks at least the second case, and can break the first as well if it does a full cleanup instead of just closing the file descriptor of the connection socket. While most cases where things break are not as clear cut as the above examples, they can and do exist. Dan0042 (Daniel DeLorme) wrote in #note-3: > byroot (Jean Boussier) wrote in #note-2: > > > Afaik the proper way to do this is to close the connection after the fork. > > > > No before. Otherwise the connection is "shared" and closing it in the children cause issues for the connections in the parent. > > It seems quite easy to demonstrate otherwise: > > ```ruby > # with Sequel and mysql2 > DB["select rand()"].first #=> {:"rand()"=>0.1878050166999968} > m = DB.pool.available_connections.first > Process.wait fork{ > IO.for_fd(m.socket).close #without this the query below would fail > } > DB["select rand()"].first #=> {:"rand()"=>0.3590592307588637} > ``` This is not a good example because it's not portable across adapters. You are relying on the database driver exposing the file descriptor (not all drivers do). Sequel doesn't have code that only closes sockets for connections, it calls a method that will actually clean up the connection state, such as rolling back active transactions, which when called on a connection in a child process will break later usage of the same connection in the parent process. Sequel recommends calling `Sequel::Database#disconnect` before forking in the common case where you are dealing with a pool of worker processes. However, even if Ruby added a general before/after fork hook, Sequel would never use it automatically, since it is impossible to determine whether doing so is desired or not. An additional issue is that not all forks are equal. Consider code such as: ```ruby DB = Sequel.postgres DB.disconnect pids = 4.times.map do fork do # worker process # ... if some_condition fork do do_something_in_worker_child exit! end end end end ``` The idea with this proposal is you could simplify things by eliminating the explicit `DB.disconnect` call before the first fork. However, doing so automatically could break things in a worker process if the second fork was called. Another issue with this proposal is that in the above example, `DB.disconnect` is only called once when called explicitly, but would be called 4 times if called implicitly. I understand why this is being requested, and I do think it could simplify common Ruby web application deployment scenarios. However, even if requires additional explicit code, I think the current approach of per-server fork hooks is better. ---------------------------------------- Feature #17795: `before_fork` and `after_fork` callback API https://bugs.ruby-lang.org/issues/17795#change-91506 * 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: #### `Process.before_fork` and `Process.after_fork` callbacks One solution would be for Ruby to expose a callback API for these two events, similar to `Kernel.at_exit`. #### 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: