Define Rack::Builder::Config, and support middleware.rackup configuring middleware based on server configuration #1720

jeremyevans · 2020-11-14T16:54:20Z

This is an alternative approach to #1718. Instead of creating a new
builder for every middleware that responds to rackup, we have a
single configuration object that stores the server's configuration,
such as whether it is multithreaded. If middleware.rackup is defined,
we call it with the configuration object as the first object, and the
remaining arguments and block that would have been passed to new.
The middleware.rackup method can return the app itself if it not
needed, call new with the remaining arguments if it is needed, or
potentially other actions for more complex cases.

To handle the very rare case where a middleware would want to
delegate to other middleware in certain server configurations, and
doesn't know whether the other middleware supports the rackup
method or not, The configuration object supports a rackup method,
which will call rackup on the middleware if defined, or new
otherwise. So if middleware A wants to use external middleware B
in a certain server configuration, middleware A's rackup method
could be something like:

  def self.rackup(config, app)
    if config.multithread?
      config.rackup(MiddlewareB, app)
    else
      new(app)
    end
  end

The configuration rackup method is also used internally to implement
the builder.

This readds Rack::Lock, using the rackup method, which only uses
the Rack::Lock middleware if the configuration indicates the
server is multithreaded.

The advantage of this approach is that it doesn't require exposing
the entire builder API to the middleware, it only exposes the
server configuration, which is all the middleware should need to
appropriately configure itself.

ioquatix · 2021-11-03T22:20:08Z

I generally like this idea, I'm not strongly in favour of any particular approach, but I would like to explore a bit more about how this works in practice. The idea of a config object is nice since it isolates the responsibilities, but it also introduces additional complexities. However, I think the net gain for users is that it simplifies more complex construction of the application and provides a very well defined interface for the kinds of things people should depend on.

The only thing I wonder about is whether we should allow config to contain user specific keys/values. I imagine that such a model could be really useful but at the expense of opening up the design to more complexity. However, the advantage would be that users could depend on a configuration context for their own usage rather than having multiple sources of truth for configuration.

ioquatix · 2022-01-23T21:47:45Z

@jeremyevans do you mind rebasing this on master?

ioquatix

Looking great. Just some points for us to discuss and iron out.

ioquatix · 2022-01-23T21:48:10Z

CHANGELOG.md

@@ -34,6 +34,10 @@ All notable changes to this project will be documented in this file. For info on

 - [[CVE-2020-8184](https://nvd.nist.gov/vuln/detail/CVE-2020-8184)] Do not allow percent-encoded cookie name to override existing cookie names. BREAKING CHANGE: Accessing cookie names that require URL encoding with decoded name no longer works. ([@fletchto99](https://github.com/fletchto99))

+### Removed


Should we also add a note regarding the addition of the new configuration class?

Sure, I can do that. Note that most users won't be using it, only middleware/server authors.

ioquatix · 2022-01-23T21:48:27Z

README.rdoc

@@ -77,7 +77,6 @@ middleware:
 * Rack::Files, for serving static files.
 * Rack::Head, for returning an empty body for HEAD requests.
 * Rack::Lint, for checking conformance to the \Rack API.
-* Rack::Lock, for serializing requests using a mutex.


We didn't end up removing this right?

Correct, I'll restore it.

ioquatix · 2022-01-23T21:49:24Z

lib/rack/builder.rb

+    # Config stores settings on what the server supports, such as whether it
+    # is multithreaded.
+    class Config
+      def initialize(multithread: true, reentrant: multithread)


I wonder if we should have a more generic interface **options so that different servers can pack different information into this configuration.

If we choose to do this, maybe we should also expose attr :options?

My idea here is that server authors can subclass this class to provide a richer API if they need to. Currently, the only use for the configuration is for concurrency. The issue with **options is that it turns typos into silent failures.

ioquatix · 2022-01-23T21:52:13Z

lib/rack/builder.rb

+
+      # Re-entrancy is a feature of event-driven servers which may perform non-blocking operations. When
+      # an operation blocks, that particular request may yield and another request may enter the application stack.
+      def reentrant?; @reentrant; end


How do you feel about this configuration option name?

I started wondering if we should call it multifiber or even replace multithread with parallel? and reentrant? with concurrent?. multithread? sounds like an implementation detail.

I think we should try to iron this interface out as it's going to be important going forward that it means exactly what we want it to mean for the servers that need to support it. We might want to add a table showing potential values for these configuration options w.r.t. servers and their respective configuration.

If multithread? is an implementation detail, so is multifiber?. parallel? is not accurate on CRuby, since Ruby code does not execute in parallel on CRuby (though that is also an implementation detail). Replacing reentrant? with concurrent? seems like a good idea to me, so I'll make that change.

I would like to get rid of multithread?, but I think it it is necessary until we drop Ruby 2.7 support. That is because on Ruby 2.7 and below, you cannot use Rack::Lock in the concurrent but not multithread case, since it uses Mutex internally.

ioquatix · 2022-01-23T21:53:53Z

lib/rack/builder.rb

+      builder = self.new(config: config)
+
+      # Create a top level scope with self as the builder instance:
+      binding = TOPLEVEL_BINDING.eval('->(builder){builder.instance_eval{binding}}').call(builder)


It's not particularly important, but in another project I moved TOPLEVEL_BINDING.eval('->(builder){builder.instance_eval{binding}}') into a separate constant, i.e.

# Top level of file: module Rack; end Rack::BUILDER_CONTEXT = ->(builder){builder.instance_eval{binding}} # ... binding = BUILDER_CONTEXT.call(builder)

@eregon this should be the same thing right?

That seems fine to me. I'd like the constant to be under Rack::Builder. So I plan to use Rack::Builder::EVAL_CONTEXT?

ioquatix · 2022-01-23T21:59:13Z

lib/rack/lock.rb

-      @mutex.unlock
-      @env[RACK_MULTITHREAD] = @old_rack_multithread
+    def self.rackup(config, app)
+      if config.multithread?


The fiber scheduler now correctly handles locks so this might be relevant no matter what? i.e. we might prefer to use reentrant? as it's currently defined.

We can do this for concurrent?, but I think only on Ruby 3+. I'll modify the code to handle that.

lib/rack/lock.rb

jeremyevans

@ioquatix I pushed a new commit and tried to respond to all of the points you raised.

jeremyevans · 2022-01-24T19:10:13Z

CHANGELOG.md

@@ -34,6 +34,10 @@ All notable changes to this project will be documented in this file. For info on

 - [[CVE-2020-8184](https://nvd.nist.gov/vuln/detail/CVE-2020-8184)] Do not allow percent-encoded cookie name to override existing cookie names. BREAKING CHANGE: Accessing cookie names that require URL encoding with decoded name no longer works. ([@fletchto99](https://github.com/fletchto99))

+### Removed


Sure, I can do that. Note that most users won't be using it, only middleware/server authors.

jeremyevans · 2022-01-24T19:10:58Z

README.rdoc

@@ -77,7 +77,6 @@ middleware:
 * Rack::Files, for serving static files.
 * Rack::Head, for returning an empty body for HEAD requests.
 * Rack::Lint, for checking conformance to the \Rack API.
-* Rack::Lock, for serializing requests using a mutex.


Correct, I'll restore it.

jeremyevans · 2022-01-24T19:14:15Z

lib/rack/builder.rb

+    # Config stores settings on what the server supports, such as whether it
+    # is multithreaded.
+    class Config
+      def initialize(multithread: true, reentrant: multithread)


My idea here is that server authors can subclass this class to provide a richer API if they need to. Currently, the only use for the configuration is for concurrency. The issue with **options is that it turns typos into silent failures.

jeremyevans · 2022-01-24T19:27:12Z

lib/rack/builder.rb

+
+      # Re-entrancy is a feature of event-driven servers which may perform non-blocking operations. When
+      # an operation blocks, that particular request may yield and another request may enter the application stack.
+      def reentrant?; @reentrant; end


If multithread? is an implementation detail, so is multifiber?. parallel? is not accurate on CRuby, since Ruby code does not execute in parallel on CRuby (though that is also an implementation detail). Replacing reentrant? with concurrent? seems like a good idea to me, so I'll make that change.

I would like to get rid of multithread?, but I think it it is necessary until we drop Ruby 2.7 support. That is because on Ruby 2.7 and below, you cannot use Rack::Lock in the concurrent but not multithread case, since it uses Mutex internally.

jeremyevans · 2022-01-24T19:33:16Z

lib/rack/builder.rb

+      builder = self.new(config: config)
+
+      # Create a top level scope with self as the builder instance:
+      binding = TOPLEVEL_BINDING.eval('->(builder){builder.instance_eval{binding}}').call(builder)


That seems fine to me. I'd like the constant to be under Rack::Builder. So I plan to use Rack::Builder::EVAL_CONTEXT?

lib/rack/lock.rb

jeremyevans · 2022-01-24T19:37:34Z

lib/rack/lock.rb

-      @mutex.unlock
-      @env[RACK_MULTITHREAD] = @old_rack_multithread
+    def self.rackup(config, app)
+      if config.multithread?


We can do this for concurrent?, but I think only on Ruby 3+. I'll modify the code to handle that.

lib/rack/builder.rb

ioquatix · 2022-01-25T00:19:31Z

lib/rack/builder.rb

@@ -31,6 +38,30 @@ module Rack
  # You can use +map+ to construct a Rack::URLMap in a convenient way.

  class Builder
+    # Config stores settings on what the server supports, such as whether it
+    # is multithreaded.


Do you think it would make sense for us to explain that we expect severs may sub-class this to provide additional details?

Also, considering this usage, does it make more sense to provide a generic options hash?

Otherwise, I imagine we might have:

def rackup(config, app) if config.respond_to?(:puma_thing) puma_thing = config.puma_thing ...

While I'm okay with the general idea of a strong interface, I'm a little concerned about how we should use it if puma/falcon/thin/unicorn start adding server-specific interfaces.

We should also consider the case where Rack is an interface rather than a gem. To this end, it feels like options is a stronger contender since it's simpler. But it's also less pleasant to use.

Basically, I prefer your design, but I think we need to consider some specific usage scenarios.

Currently, the configuration would only be used for Rack::Lock. I'm not aware of any other use case, so making it more flexible and prone to silent failures doesn't seem like a good trade-off. If you have ideas for how this will be used by servers, that information would definitely be helpful. That said, I'm not strongly against the options hash approach.

Rack for the most part tries to separate "Rack the SPEC" and "Rack the GEM". This configuration interface touches both worlds. I personally think Rack would benefit from richer interfaces, but I also appreciate the original design which was servers could follow "Rack the SPEC" without depending on "Rack the GEM". IIRC, Passenger does not pull in "Rack the GEM" and instead just follows the spec. In that case, @FooBarWidget would need a bespoke implementation of Rack::Builder::Config but it's not specified anywhere except for the implementation in the GEM. Maybe this is a longer term project - we should define config.ru - i.e. use, run, the context for evaluation, the builder and config interfaces, etc.

Frankly it feels a lot harder to define an interface like Rack::Builder::Config - the building blocks we've used for "Rack the SPEC" has been Hash, Array, String, and other simple types. In any such spec, I guess we would leave the actual class as anonymous and just define the interface, e.g.

Your application may respond to `rackup` in which case it will be invoked with a configuration object which contains at least the following interface: multithread? -> true | false concurrent? -> true | false

I guess this all hinges on how much we define config.ru and Rack::Builder as "SPEC" vs "GEM". cc @tenderlove your input would be useful too.

I think for the middleware users and server implementers using hashes for this config would be better. It is easier to check for a hash value than it is to check if the config object respond to some method the only some server configs provide.

For what I get of this implementation, the only reason why a hash as config would not work right now is because the object allows servers to implement the rackup method, but do we see them needing this kind of feature?

Using Rack::Builder directly has the same issues that using Rack::Builder::Config does in terms of SPEC. I agree from a SPEC perspective, rack avoids custom objects. However, config.ru was never part of SPEC (which is only regarding the rack protocol), so I'm not sure it applies.

The issue with using a plain hash is that it won't have the equivalent of Config#rackup, so middleware cannot pass the configuration to other middleware loaded by that middleware. However, maybe there are no such middleware that actually need that. Switching to a plain hash seems fine, I can work on that change if that's the direction we want to go.

However, maybe we should rethink the idea of middleware autoconfiguration based on configuration parameters? Is it really needed? Maybe we can just have Rack::Lock always lock, and users who know their apps are not concurrency-safe can use it. If they use it on a non-concurrent webserver (e.g. Unicorn), it will use a lock when it doesn't need to, but uncontested mutex is not slow, so there is no real problem with doing so. What are your thoughts on abandoning the idea of middleware configuration? We would still remove rack.multithread/rack.multiprocess/rack.run_once.

What are your thoughts on abandoning the idea of middleware configuration? We would still remove rack.multithread/rack.multiprocess/rack.run_once.

I'm positive for that idea. To be fair the only middleware that I saw which uses this is Rack::Lock, which Rails doesn't include anymore by default and uses it own config config.allow_concurrency to configure it.

Why don't we still expose Builder#options for server configuration/specific details at load time. Otherwise there is no per-builder side channel for server configuration/details at all. We don't need to handle the rackup model in the same PR, if it's too hard, we can drop it. Users can write use MyMiddleware.rackup(self) if they want.

I don't see the need for a per-builder side channel. The complexity isn't worth it, IMO. Looks like @rafaelfranca agrees. So unless another committer is in favor of adding it, we should probably just commit the removal of rack.multithread/rack.multiprocess/rack.run_once.

Okay, let's do that to start with.

These variables generally come too late to be useful. Removed `Rack::Lock` which depends on these variables.

This adds Rack::Builder::Config, which stores information on the server's configuration, such as whether it is multithreaded or supports reentrancy. Middleware can use the configuration by defining a rackup method in addition to a new method. If the rackup method is defined, it is called instead of the new method, with the configuration as the first argument and with the remaining arguments and block the same as what would be passed to new. In cases where the server's configuration indicates the middleware is not needed, the middleware rackup method can be just return the app itself. To handle the very rare case where a middleware would want to delegate to other middleware in certain server configurations, and doesn't know whether the other middleware supports the rackup method or not, The configuration object supports a rackup method, which will call rackup on the middleware if defined, or new otherwise. So if middleware A wants to use external middleware B in a certain server configuration, middleware A's rackup method could be something like: def self.rackup(config, app) if config.multithread? config.rackup(MiddlewareB, app) else new(app) end end The configuration rackup method is also used internally to implement the builder. This readds Rack::Lock, using the rackup method, which only uses the Rack::Lock middleware if the configuration indicates the server is multithreaded. The advantage of this approach is that it doesn't require exposing the entire builder API to the middleware, it only exposes the server configuration, which is all the middleware should need to appropriately configure itself.

Avoids the need to use TOPLEVEL_BINDING.

… on Ruby 2 Ruby 2 Mutex only works for multiple threads, not multiple fibers. Such apps are probably still broken at runtime unless they are using their own locks, but this matches the Rack 2 behavior.

This correctly doesn't release the lock until after the body is closed. Restore the related tests as well. Just make changes to avoid use of rack.multithread.

ioquatix

Great.

When using Rack >= 3.0.0 you get this error message on boot an app with Flipper UI : ``` warning: Rack::File is deprecated and will be removed in Rack 3.1 ``` This PR detects if `Rack::Files` is defined and tries to use it instead. Source : rack/rack#1720

These constants were removed from Rack in rack/rack#1720

jeremyevans mentioned this pull request Nov 14, 2020

Remove per-request "server implementation details" and provide a consistent way to expose this to middleware during build time. #1718

Closed

jeremyevans requested a review from ioquatix November 3, 2021 21:27

ioquatix requested changes Jan 23, 2022

View reviewed changes

jeremyevans force-pushed the middleware-rackup branch from b67c697 to 4846141 Compare January 24, 2022 19:57

jeremyevans commented Jan 24, 2022

View reviewed changes

jeremyevans requested a review from ioquatix January 24, 2022 20:49

ioquatix reviewed Jan 25, 2022

View reviewed changes

lib/rack/builder.rb Outdated Show resolved Hide resolved

ioquatix requested changes Jan 25, 2022

View reviewed changes

ioquatix mentioned this pull request Jan 25, 2022

How to handle backwards compatibility #1599

Closed

ioquatix and others added 11 commits January 26, 2022 16:37

Remove rack.multithread/rack.multiprocess/rack.run_once.

e9e8ec3

These variables generally come too late to be useful. Removed `Rack::Lock` which depends on these variables.

Always use mutex. Uncontested mutex is not slow.

fb677a0

Fixup rack lint

59a14b5

Fixup CHANGELOG -1st commit

174b412

Add Rack::Builder::EVAL_CONTEXT

64785cd

Avoids the need to use TOPLEVEL_BINDING.

Rename reentrant to concurrent? in Rack::Builder::Config

ece9dc9

Remove unused logger require in lib/rack/lock.rb

80f0a14

Don't use Rack::Lock for concurrent but not multithread configuration…

4d3bce0

… on Ruby 2 Ruby 2 Mutex only works for multiple threads, not multiple fibers. Such apps are probably still broken at runtime unless they are using their own locks, but this matches the Rack 2 behavior.

Remove Rack::Builder::Config

38baae6

Restore Rack::Lock implementation

a8a0459

This correctly doesn't release the lock until after the body is closed. Restore the related tests as well. Just make changes to avoid use of rack.multithread.

jeremyevans force-pushed the middleware-rackup branch from 4846141 to a8a0459 Compare January 27, 2022 01:34

ioquatix approved these changes Jan 27, 2022

View reviewed changes

jeremyevans merged commit 0f8bb0f into rack:master Jan 27, 2022

jeremyevans mentioned this pull request Feb 4, 2022

Managing environments. #1546

Closed

ioquatix mentioned this pull request Apr 6, 2022

Consider introducing env['rack.request.headers'] for original headers before transforming them. #1841

Closed

czj mentioned this pull request Nov 13, 2023

Handle deprecation of Rack::File in Rack 3.1 flippercloud/flipper#773

Merged

albertski added a commit to albertski/lamby that referenced this pull request Jan 26, 2024

Remove uninitialized constants

504c94c

These constants were removed from Rack in rack/rack#1720

albertski mentioned this pull request Jan 26, 2024

Remove uninitialized constants rails-lambda/lamby#173

Merged

jeremiahlukus pushed a commit to rails-lambda/lamby that referenced this pull request Jun 25, 2024

Remove uninitialized constants (#173)

b786b5c

These constants were removed from Rack in rack/rack#1720

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Define Rack::Builder::Config, and support middleware.rackup configuring middleware based on server configuration #1720

Define Rack::Builder::Config, and support middleware.rackup configuring middleware based on server configuration #1720

jeremyevans commented Nov 14, 2020

ioquatix commented Nov 3, 2021

ioquatix commented Jan 23, 2022

ioquatix left a comment

ioquatix Jan 23, 2022

jeremyevans Jan 24, 2022

ioquatix Jan 23, 2022

jeremyevans Jan 24, 2022

ioquatix Jan 23, 2022

jeremyevans Jan 24, 2022

ioquatix Jan 23, 2022

jeremyevans Jan 24, 2022

ioquatix Jan 23, 2022

jeremyevans Jan 24, 2022

ioquatix Jan 23, 2022

jeremyevans Jan 24, 2022

jeremyevans left a comment

jeremyevans Jan 24, 2022

jeremyevans Jan 24, 2022

jeremyevans Jan 24, 2022

jeremyevans Jan 24, 2022

jeremyevans Jan 24, 2022

jeremyevans Jan 24, 2022

ioquatix Jan 25, 2022

ioquatix Jan 25, 2022

jeremyevans Jan 25, 2022

ioquatix Jan 25, 2022

rafaelfranca Jan 26, 2022

jeremyevans Jan 26, 2022

rafaelfranca Jan 26, 2022

ioquatix Jan 26, 2022 •

edited

Loading

jeremyevans Jan 26, 2022

ioquatix Jan 27, 2022

ioquatix left a comment

		@@ -34,6 +34,10 @@ All notable changes to this project will be documented in this file. For info on

		- [[CVE-2020-8184](https://nvd.nist.gov/vuln/detail/CVE-2020-8184)] Do not allow percent-encoded cookie name to override existing cookie names. BREAKING CHANGE: Accessing cookie names that require URL encoding with decoded name no longer works. ([@fletchto99](https://github.com/fletchto99))

		### Removed

Define Rack::Builder::Config, and support middleware.rackup configuring middleware based on server configuration #1720

Define Rack::Builder::Config, and support middleware.rackup configuring middleware based on server configuration #1720

Conversation

jeremyevans commented Nov 14, 2020

ioquatix commented Nov 3, 2021

ioquatix commented Jan 23, 2022

ioquatix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyevans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioquatix Jan 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioquatix left a comment

Choose a reason for hiding this comment

ioquatix Jan 26, 2022 •

edited

Loading