Linux's epoll+accept queue is fundamentally LIFO (see a good writeup at https://blog.cloudflare.com/the-sad-state-of-linux-socket-balancing/).

Because of this, both Unicorn and Pitchfork don't peroperly balance load between workers, unless the deployment is at capacity, the first workers will handle disproportionately more work.

In some ways this behavior can be useful, but in other it may be undesirable. Most notably in can create a situation where some of the workers are only used when there is a spike of traffic, and when that spike happen, it hit colder workers.

Pitchfork helps with that cold worker issue thanks to reforking, however the first few requests after reforking are likely to hit page fault, so they still have some (smaller) cold worker problem.

We could explore opening multiple TCP servers with SO_REUSEPORT to split the load evenly between subgroups of workers. The downside if that this would create a round-robin between each group, so if one group get multiple much slower requests it may spike latency.

One way I'd like to explore would be to have intertwined groups, e.g.:

• Say 32 workers (0, 1, ...)
• 8 re-usedports (A, B, C,...)

We could do something where:

• Workers 0..7 listen to A
• Workers 4..11 listen to B
• Workers 8...15 listen to C

This way each worker listen into multiple request pools (2 in the example, but could be more).

I think such setup could be a good compromise between fairness and tail latency.

ERROR -- [Pitchfork]: master loop error: undefined method `sendmsg_nonblock' for nil:NilClass (NoMethodError)
ERROR -- [Pitchfork]: /artifacts/bundle/ruby/3.2.0/bundler/gems/pitchfork-0290e18bd75e/lib/pitchfork/worker.rb:219:in `send_message_nonblock'
ERROR -- [Pitchfork]: /artifacts/bundle/ruby/3.2.0/bundler/gems/pitchfork-0290e18bd75e/lib/pitchfork/worker.rb:135:in `soft_kill'
ERROR -- [Pitchfork]: /artifacts/bundle/ruby/3.2.0/bundler/gems/pitchfork-0290e18bd75e/lib/pitchfork/http_server.rb:633:in `block in restart_outdated_workers'
ERROR -- [Pitchfork]: /artifacts/bundle/ruby/3.2.0/bundler/gems/pitchfork-0290e18bd75e/lib/pitchfork/http_server.rb:631:in `each'
ERROR -- [Pitchfork]: /artifacts/bundle/ruby/3.2.0/bundler/gems/pitchfork-0290e18bd75e/lib/pitchfork/http_server.rb:631:in `restart_outdated_workers'
ERROR -- [Pitchfork]: /artifacts/bundle/ruby/3.2.0/bundler/gems/pitchfork-0290e18bd75e/lib/pitchfork/http_server.rb:339:in `monitor_loop'
ERROR -- [Pitchfork]: /artifacts/bundle/ruby/3.2.0/bundler/gems/pitchfork-0290e18bd75e/lib/pitchfork/http_server.rb:305:in `join'
ERROR -- [Pitchfork]: /artifacts/bundle/ruby/3.2.0/bundler/gems/pitchfork-0290e18bd75e/exe/pitchfork:109:in `<top (required)>'

This happened after a manual refork, we may have a race condition somewhere. To be investigated.

Hey, your README is a delight by the way. It was easy to understand, it identified provenance; approach. I Really really enjoyed it. Please view my impertinent question remembering that 🙏 .

Do you have plans or a policy to upstream to unicorn; and also on what and how you'll take from unicorn. Managing divergent projects can be such a large source of effort, I'm genuinely curious to know if you have a blog dedicated to pitchfork, where I might satisfy my curious nature about the intentions, how that is going, etc.

Have a great weekend, and it's very cool you are trying new things in the open. 👍 🏆

This may or may not be a good idea, but it would be interesting to have an option to close all FDs in the mold.

We can list all FDs via /proc or use close_range(2). We'd only want to keep the binds and the command socket, all other FDs could be closed.

Most Ruby libraries are able to deal gracefully with a closed connection, so it might be a decent transition workaround for some libraries that aren't yet fork safe.

Alternative

• The POSIX spec has a FD_CLOFORK FD attribute, but it's not implemenented by Linux, only macOS, Solaris and some exotic unixes.

I was load testing pitchfork with 64 workers, restarting them one by one was quite slow.

For every rollup we may need to wait for as long as the request timeout, so a full rollup may take as long as workers_count * request_timeout.

We should either have a setting that allow to define how many workers may be rolling out at a time, or a proportion of workers that can be rolling out (e.g. 10%).

Rack 3.1 is removing the Rack::Chunked middleware and expect servers to implement it themselves.

I'm curious why PR_SET_CHILD_SUBREAPER is a hard requirement for reforking in pitchfork.

From what I've read in the Puma implementation code and documentation, reforking there can happen on any unix-like system. It doesn't seem to require or utilize subreaping.

In pitchfork, if REFORKING_AVAILABLE is false no reforking happens at all and it's just a multiprocess preloading server. I can guess at some benefits of this approach:

• Some kind of issues with nesting?
• Ability to create molds of future generations that maybe isn't possible in the non-subreaping approach?

But the documentation and code mostly tells me what the requirements are, but not why they are a necessity. I'm looking into writing a generalized reforking worker pool, so i'm curious what research led to a subreaper focused implementation.

Thanks!

After each reforking, workers have 3 extra frames, e.g. from a gen 4 worker:

from lib/pitchfork/http_server.rb:625:in `process_client'
from lib/pitchfork/http_server.rb:727:in `worker_loop'
from lib/pitchfork/http_server.rb:467:in `block in spawn_worker'
from lib/pitchfork/http_server.rb:456:in `fork'
from lib/pitchfork/http_server.rb:456:in `spawn_worker'
from lib/pitchfork/http_server.rb:760:in `mold_loop'
from lib/pitchfork/http_server.rb:469:in `block in spawn_worker'
from lib/pitchfork/http_server.rb:456:in `fork'
from lib/pitchfork/http_server.rb:456:in `spawn_worker'
from lib/pitchfork/http_server.rb:760:in `mold_loop'
from lib/pitchfork/http_server.rb:469:in `block in spawn_worker'
from lib/pitchfork/http_server.rb:456:in `fork'
from lib/pitchfork/http_server.rb:456:in `spawn_worker'
from lib/pitchfork/http_server.rb:760:in `mold_loop'
from lib/pitchfork/http_server.rb:469:in `block in spawn_worker'
from lib/pitchfork/http_server.rb:456:in `fork'
from lib/pitchfork/http_server.rb:456:in `spawn_worker'
from lib/pitchfork/http_server.rb:760:in `mold_loop'
from lib/pitchfork/http_server.rb:490:in `block in spawn_initial_mold'
from lib/pitchfork/http_server.rb:486:in `fork'
from lib/pitchfork/http_server.rb:486:in `spawn_initial_mold'
from lib/pitchfork/http_server.rb:125:in `start'

Not a big deal unless you end up reforking dozens or hundreds of times, but would be good to address, as stack space isn't unlimited.

One way could be to fork from a new thread, downside is that we may lose fiber/thread variables. I can't think of a case where that would matter, but it might.

We've been using pitchfork now in production for a week and on the whole it's been great.

One issue we've noticed though is randomly and in-frequently (happened 3 times in a week) a forked worker mysteriously loses all/some of the i18n keys and we get translations missing all over the place. We're not using anything fancy, just regular Rails 7 i18n. Our setup is 5-10 pods, 28 workers in each pod, handling ~50rps.

Before trying to investigate deeper, I wanted to check if anyone had experienced anything similar or anyone had any suggestions of where we should start looking.

Pitchfork config:

# Sets the number of desired worker processes. Each worker process will serve exactly one client at a time.
worker_processes ENV.fetch('WEB_CONCURRENCY', 1).to_i

 # listen to port 3000 on all TCP interfaces
listen ENV.fetch('PORT', 3000).to_i, tcp_nopush: true

# nuke workers after 120 seconds instead of 60 seconds (the default)
timeout 120

# Enable this flag to have pitchfork test client connections by writing the
# beginning of the HTTP headers before calling the application.  This
# prevents calling the application for connections that have disconnected
# while queued.  This is only guaranteed to detect clients on the same
# host pitchfork runs on, and unlikely to detect disconnects even on a
# fast LAN.
check_client_connection false

# Sets a number of requests threshold for triggering an automatic refork.
# The limit is per-worker, for instance with refork_after [50] a refork is triggered once at least one worker processed 50 requests.
refork_after [500, 750, 1000, 1200, 1400, 1800, 2000, 2200, 2400, 2600, 2800, 3000, 3200, 3400, 3600, 3800, 4000, 4200, 4400, 4600, false]

# Called in the context of the mold after it has been spawned.
# Its usage is similar to a before_fork callback found on other servers but it is called once on promotion rather than before forking each worker.
# For most protocols connections can be closed after fork, but some stateful protocols require to close connections before fork.
# That is the case for instance of many SQL databases protocols.
# This is also the callback in which memory optimizations, such as heap compaction should be done.
after_mold_fork do |server, mold|
  ActiveRecord::Base.connection.disconnect! if defined?(ActiveRecord::Base)
end

# Called in the worker after forking.
# Generally used to close inherited connections or to restart backgrounds threads for libraries that don't do it automatically.
after_worker_fork do |server, worker|
  ActiveRecord::Base.establish_connection if defined?(ActiveRecord::Base)
  SemanticLogger.reopen if defined?(SemanticLogger)
end

Right now, it's the master process that keeps an eye on workers and chose to promote one of them when some condition is reached.

Pros:

• It's somewhat easy to make it race-condition free because only a single threaded process makes the decision.

Cons:

• It prevent the master from sleeping for prolonged amount of time, like in unicorn, as we have to wake up frequently to check if the promotion condition was reached. That's not great.
• The condition has to be checked on all workers sequentially, which means it doesn't scale very well with the number of workers.
• We don't really know if the worker is idle of busy with a request that will still take a very long time. So we may end up selecting a busy worker that will end up timing out. We currently handle this case, but it delays the new generation.
• It makes it harder to allow custom mold selection implementations, as the master doesn't load the app.

Since that's a lot of cons, I think we should explore moving this logic into the workers.

Pros:

• Workers would only be considered for promotion when they are idle (or just finished a request).
• That reduce load on the master.

Cons:

• We need some shared mutex/semaphore to make sure only one worker is promoted, but that should be doable with a Raindrop atomic counter.

The logic would be something like:

def check_promotion
  if condition_met
    if acquire_promotion_lock
      start_promotion # start by notifying master, promote, confirm to master.
    else
      # somehow debounce promotion_check ?
      # e.g. if we continue to use request_count, we can do `request_limit *= 1.20`
    end
  end
end

Hi, first I would like to thank you for amazing work!

I've rails application with sequel-rails gem. And sometimes (rare) on high traffic refork I'm getting this error:

PG::ConnectionBad: PQconsumeInput() server closed the connection unexpectedly (PG::ConnectionBad)
	This probably means the server terminated abnormally
	before or while processing the request.

  from sequel (5.74.0) lib/sequel/adapters/postgres.rb:171:in `exec'
  from sequel (5.74.0) lib/sequel/adapters/postgres.rb:171:in `block in execute_query'
  from sequel-rails (1.2.1) lib/sequel_rails/sequel/database/active_support_notification.rb:17:in `block in log_connection_yield'
  from activesupport (7.1.2) lib/active_support/notifications.rb:206:in `block in instrument'
  from activesupport (7.1.2) lib/active_support/notifications/instrumenter.rb:58:in `instrument'
  from activesupport (7.1.2) lib/active_support/notifications.rb:206:in `instrument'
  from sequel-rails (1.2.1) lib/sequel_rails/sequel/database/active_support_notification.rb:11:in `log_connection_yield'
...

I've implemented disconnect on mold fork. Here my config/pitchfork.rb

# frozen_string_literal: true

worker_processes ENV.fetch("PITCHFORK_WORKERS", 1).to_i
listen 3000

refork_after [50, 100, 1000]

after_mold_fork do |_server, _mold|
  Sequel::DATABASES.each(&:disconnect)
end

after_worker_fork do |_server, _worker|
  SemanticLogger.reopen
end

Just want a trace/exception in the rails process to do the error handling/reporting to debug/diagnose timeouts. Thanks!

In our development environment, rake ragel fails and is unable to compile pitchfork_http.rl.

Reproduce

bin/dev-console
bundle install
rake ragel

which will throw

* compiling pitchfork_http.rl
pitchfork_http_common.rl:7:2: bad write statement
pitchfork_http_common.rl:7:4: at token TK_MiddleLocalError: parse error
rake aborted!
NameError: undefined local variable or method `ragel_file' for main:Object

    system("ragel", "-G2", "pitchfork_http.rl", "-o", "pitchfork_http.c") or raise "ragel #{ragel_file} failed"
                                                                                            ^^^^^^^^^^
/app/rakefile:65:in `block (2 levels) in <top (required)>'
/app/rakefile:63:in `chdir'
/app/rakefile:63:in `block in <top (required)>'

The trace is mostly a red-herring, because ragel_file isn't defined. This is fixed in 778f4d0.

The underlying problem still exists which is that

ragel -G2 pitchfork_http.rl -o pitchfork_http.c

fails with

pitchfork_http_common.rl:7:2: bad write statement
pitchfork_http_common.rl:7:4: at token TK_MiddleLocalError: parse error

This isn't an immediate problem because we have the pitchfork_http_common.c checked in, but if we re-sync the HTTP protocol from upstream, we'll need to regenerate the file, and this may become a problem then.

Any thing immediately stand out @casperisfine?

Current investigation

I wanted to make sure that renaming wasn't the root of the issue so I reverted back and still see the same issue. We also haven't made any actual changes to the extension, so my guess is there's some discrepancy when porting over the setup from the GNUMakefile.

Right now if the callback raise, pitchfork can end up in a fork loop that can easily bring down a server.

We need to more cleanly handle a situation where the mold die when calling one of the callbacks.

After bundle installing pitchfork, when I try to run it I get the following error:

bundle exec pitchfork

11: from /usr/local/cache/ruby/2.7.0/bin/pitchfork:23:in `<top (required)>'
10: from /usr/local/cache/ruby/2.7.0/bin/pitchfork:23:in `load'
9: from /usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/exe/pitchfork:3:in `<top (required)>'
8: from /usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/exe/pitchfork:3:in `require'
7: from /usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/lib/pitchfork/launcher.rb:9:in `<top (required)>'
6: from /usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/lib/pitchfork/launcher.rb:9:in `require'
5: from /usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/lib/pitchfork.rb:156:in `<top (required)>'
4: from /usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/lib/pitchfork.rb:156:in `each'
3: from /usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/lib/pitchfork.rb:157:in `block in <top (required)>'
2: from /usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/lib/pitchfork.rb:157:in `require_relative'
1: from /usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/lib/pitchfork/http_parser.rb:4:in `<top (required)>'
/usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/lib/pitchfork/http_parser.rb:4:in `require': cannot load such file -- pitchfork/pitchfork_http (LoadError)

Updating http_parser.rb to require_relative fixes the error, which then leads to the next error:

# lib/pitchfork/http_parser.rb

# require 'pitchfork/pitchfork_http'
require_relative '../pitchfork_http'

bundle exec pitchfork

/usr/local/cache/ruby/2.7.0/gems/pitchfork-0.1.0/lib/pitchfork/http_server.rb:2:in `require': cannot load such file -- pitchfork/pitchfork_http (LoadError)

Applying require_relative inside of http_server.rb fixes that error, and the server then starts.

# lib/pitchfork/http_server.rb

# require 'pitchfork/pitchfork_http'
require_relative '../pitchfork_http'

This seems to be because in the gem source, pitchfork_http is in lib/pitchfork_http (pitchfork_http.so on docker linux, pitchfork_http.bundle on mac os). But if I pull down the project and run bundle exec rake compile, pitchfork_http gets added directly to lib/pitchfork/pitchfork_http. The require works properly in this case, and the majority of the specs work as well.

Is this intentional and I have something misconfigured, or is there a problem with the published gem?

Awesome project! 🏅

From the README:

pitchfork will bind to all interfaces on TCP port 8080 by default.

Since a --listen flag already exist, I'd suggest binding only to localhost (127.0.0.1) by default.

I know this is mostly used internally right now, but it's such a common thing for webservers to accidentally bind to a public interface, for example on developer machines, unknowingly exposing an app to the internet.

Problem

A small downside of the current approach, is that we load the app inside the server before forking the first generation of workers.

The benefits are:

• It's simpler.
• It allows to work as a simple unicorn style server if you don't use reforking, meaning it doesn't need Linux to work.
• If the application crashes on boot, that happens synchronously, making it super easy to just let the master process crash on boot.
• It doesn't require PR_SET_CHILD_SUBREAPER, so it makes the basic pitchfork features testable on any Unix-like (e.g. macOS).

The downsides are:

• The master process has all of the application code loaded, and since it will never be executed nor reforked, the master end up much bigger than it needs to be, with a fairly bad CoW performance. If you run a lots of workers, it probably doesn't matter, but with a smaller number of workers this is quite visible.
• We have two ways of spawning workers. Not a huge deal, but limiting the number of codepaths is always good.

Idea

We could instead not load the app in the master, and instead of directly spawning the first generation of workers, we'd spawn an initial mold, and have it load the application for us and then spawn the first generation of workers.

The benefits are:

• The master becomes very small.
• The application code can't conflict with the master by registering signal traps and such
• We can consider that a mold always exist, which might simplify the code quite a bit.

The downside are:

• We either break basic functionality on non-Linux systems, or complexify the code a bit more.
• We need to properly handle the mold crashing on boot. Not hard, but still more code than just booting the app from the master.

Context

A fundamental problem Pitchfork has to deal with is that both POSIX and Linux don't quite support running anything but async-signal safe function after a fork().

In practice, as long as you never spawned any background thread, you are fine. But many ruby applications and gems do spawn threads, and in presence of such background threads if we happen to fork at the wrong time, it can result in a sub process that is in an unrecoverable state.

The typical case is forking while a background thread hold a lock, in the child this lock will remain locked and trying to access it will dead lock.

For instance this can happen with OpenSSL 3:

    [/usr/lib/x86_64-linux-gnu/libc.so.6] pthread_rwlock_wrlock
    [/usr/lib/x86_64-linux-gnu/libcrypto.so.3] CRYPTO_THREAD_write_lock
    [/usr/lib/x86_64-linux-gnu/libcrypto.so.3] CRYPTO_alloc_ex_data
    [/usr/lib/x86_64-linux-gnu/libcrypto.so.3] OPENSSL_thread_stop
    [/usr/lib/x86_64-linux-gnu/libcrypto.so.3] OPENSSL_cleanup
    [/usr/lib/x86_64-linux-gnu/libc.so.6] secure_getenv

So any background thread that use a SSL connection may break reforking.

That's what Pitchfork.prevent_fork is for, but still, we should try to handle such scenario as gracefully as possible.

Action Plan

• If we detect such case we should terminate the affected process.
• Ideally we replace that process with a new one, but if for some reason we can't, we should gracefully terminate the whole server (last resort).
• We should consider "reverting" #42.
  • Spawning the new mold out of a worker has the nice property of not impacting capacity as much
  • However that fork is risky because workers are even more likely than molds to have background threads.
  • We should probably warn for every thread in the mold (Puma does something similar)
  • (optional) We could provide a way to run background threads in a dedicated process outside the mold.
  • Provide a callback to validate post-fork processes
    • Maybe even validate the usual suspects by default (OpenSSL)

This remain to be seen whether we'll actually need this, but would be worth exploring.

The assumption is that the first few generation will invalidate shared pages rather quickly, as such the peak memory usage at first might get quite high.

A possible solution to this would be to start with a lower number of workers to compensate for the lower CoW effectiveness, and then increase the number of workers of processes with each new generation.

This would mean pitchfork needs a bit of a warm up before achieving peak throughput.

The before_fork / after_fork isn't quite enough, the one event that is very important is after a worker has been promoted to mold.

That is where you'll want to close FDs, run major GC, compact memory, etc.

Shamelessly copied from here puma/puma#3304

I started looking into the failing builds on master out of curiosity, didn't come to a solution but thought I'd share some findings. I'm happy to pair on this @casperisfine, if you haven't got to it by EST.

I haven't been able to reproduce the failing test locally: t0300-no-default-middleware.sh passes for me using the image specified in the Dockerfile.

Based on that, I started exploring what would cause a discrepancy between local and CI:

1. Perhaps there's variance between how the bin/dev-console and ci are configured

I made sure the same packages are installed, which didn't change anything. Aside from that, I couldn't find any setup differences in terms of configuration.

2. I was curious what the value of the http_code was for the test in CI: it returns 00 (as opposed to the expected 42).

3. I started to consider that maybe the way the test is written (setting --no-default-middleware, using a purposely incorrect response (integer) and relying on the fact that Rack::Lint is no longer present) is obfuscating the real problem, although I default to this being unlikely given the test seemingly passes locally 🤔

I've just started to look into possible discrepancies in how RACK_ENV is set across environments, and perhaps Rack::Lint isn't being removed in CI?

In any case, I'm not sure if there's much value in those threads. Curious to see where the problem lies.

The current integration tests are inherited from Unicorn. They're very hard to debug, especially if they fail on CI.

It would be good to port them to minitest, with Ruby instead of Bash, it would be much easier to produce readable and actionable error reports.

• t0000-http-basic
• t0002-parser-error
• t0004-heartbeat-timeout
• t0009-broken-app
• t0010-reap-logging
• t0013-rewindable-input-false
• t0014-rewindable-input-true
• t0015-configurator-internals
• t0018-write-on-close
• t0019-max_header_len
• t0020-at_exit-handler
• t0022-listener_names-preload_app
• t0100-rack-input-tests
• t0116-client_body_buffer_size
• t0300-no-default-middleware
• t2000-promote-worker
• t9000-preread-input

Ref: #55

It's the monitor that forward the reforking command, and the monitor doesn't know which worker is fork safe and which is not.

The worker should ignore that command if it's no longer fork safe.

We could also consider putting that info in shared memory, but that is a bit overkill for something that is mostly a debugging tool.