Rack: 2.2.x Rack::Lint severe performance regression

[ziggythehamster]

In version 2.2.x, Rack::Lint does this check in _call:

      assert("response #{ary.inspect} is not an Array , but #{ary.class}") {
        ary.kind_of? Array
      }
      assert("response array #{ary.inspect} has #{ary.size} elements instead of 3") {
        ary.size == 3
      }

_Even if_ the assertion is successful, the result of @app.call(env) is inspected, allocated, and dropped into a string that will never be used. Twice. For every middleware being linted. @app.call(env) returns an array of status (no big deal), headers (this is rather large once you've got stuff like Devise loaded), and body (hundreds to thousands of kilobytes!). So, Rack is allocating RAM for the response body at least three times, but probably dozens of times.

This resulted in this performance regression of our large production application (purple is "Middleware"):

I ran a transaction trace and found that each of the ~15 middlewares eats up about 1.2% of the entire request time in #inspect, for a total of 16%.

We've downgraded to 2.1.2 for the time being. I'm not sure if you actually need to inspect the array, but if you do, I'd suggest only doing it in development.

[jeremy]

Nice to improve this for its own sake; however, production performance is not a goal of Rack::Lint. It's to validate conformance to the Rack SPEC. Generally recommend you do not use it in production. Please do open a perf improvement PR if you're interested in pursuing!

[ziggythehamster]

I'm not actually intentionally loading Rack::Lint - I assume a dependency is, but the only one which I can definitely tell has this ability is Unicorn, and Unicorn is not configured to do so. I'll see if I can hunt down what is loading Rack::Lint.

[ioquatix]

We should definitely fix this.

[ziggythehamster]

It basically has to be Unicorn doing this, but I'll be damned if I can figure out why. I'm not setting RACK_ENV (documented to load no middleware), and yet it insists on loading Rack::Lint anyways... I guess I'll need to add --no-default-middleware and reconfigure Rails to load the ones it needs to that Unicorn is loading for me.

$ bundle exec rails middleware                                                                                                                                                     
use Rack::Cors
use Rack::Sendfile
use ActionDispatch::Static
use ActionDispatch::Executor
use Rack::Runtime
use Rack::MethodOverride
use ActionDispatch::RequestId
use RequestStore::Middleware
use ActionDispatch::RemoteIp
use Rails::Rack::Logger
use HealthCheck
use ActionDispatch::Flash
use ActionDispatch::ShowExceptions
use ActionDispatch::DebugExceptions
use Rollbar::Middleware::Rails::RollbarMiddleware
use ActionDispatch::Reloader
use ActionDispatch::Callbacks
use ActiveRecord::Migration::CheckPending
use ActionDispatch::Cookies
use Rack::Head
use Rack::ConditionalGet
use Rack::OptimisticConcurrencyControl
use Rack::ETag
use Warden::Manager
use Bullet::Rack
use Rack::ResponseHeaders
use Rack::ContentMd5
run MY_APP::Application.routes

[ioquatix]

Ziggy can you make a PR removing ary.inspect. I'll merge and release.. there is another bug fix I need to attend to today, so if you can do that now, that would make it easier for me to manage the next release sooner rather than later.

[jeremy]

@ziggythehamster Unicorn uses Rack::Lint when RACK_ENV is development. RACK_ENV defaults to development when invoked via bin/unicorn and -E/--env is not provided. Recommend setting that to none or deployment as well as providing --no_default_middleware.

[ziggythehamster]

@ioquatix incoming shortly

@jeremy I'm aware of that, but I thought I was setting RACK_ENV to equal RAILS_ENV, and turns out that was a stupid assumption because I was _not_ doing that! 😳 So, we've found my root cause: assuming I did something that I didn't. I'll set it to deployment.

[nateberkopec]

Didn't we just talk about RACK_ENV vs RAILS_ENV on this project last week re: something else?

[ioquatix]

Yes, and I made a gem to deal with it:

https://github.com/socketry/variant

The gist of it is, *_ENV is not a great convention. Because there is no well defined relationship between _ENV variables and additionally, no standard framework for interpreting them.

The (tiny) gem solves all those problems. In production you just need VARIANT=production and everything else should work. If you want to change specific variants, you can write DATABASE_VARIANT=production-replica or whatever else you want. In your code, just use Variant.for(:database) which uses the default variant, or a specific one if specified.

While I haven't made it the default for falcon... I'm considering VARIANT=production to be the default in falcon host which is designed for production which can avoid issues like this. Personally, I think removing RACK_ENV would reduce complexity for users of rack. There is no reason to have it if we remove the Rack::Server/rackup implementation and additionally enhance Rack::Builder to support typical use cases (e.g. #production?).