Managing environments. #1546
Comments
|
One more question I have, I was looking at the default middleware. def default_middleware_by_environment
m = Hash.new {|h, k| h[k] = []}
m["deployment"] = [
[Rack::ContentLength],
logging_middleware,
[Rack::TempfileReaper]
]
m["development"] = [
[Rack::ContentLength],
logging_middleware,
[Rack::ShowExceptions],
[Rack::Lint],
[Rack::TempfileReaper]
]
m
end
In Looking at If we can figure out what the long term vision for Rack looks like, and how we want servers to integrate with it, I think we'd proabably be able to answer some of these questions. |
|
Here is an example of part of one of my own Interestingly enough, we could actually |
|
What's even more crazy, is that puma includes an entire copy of |
|
Going over the various proposals in this issue:
This seems fine to me. However, we should definitely keep backwards compatibility and continue to use
I don't see the benefits of this, but I also don't maintain a webserver.
This middleware doesn't overwrite an existing incorrect Content-Length, so it doesn't help people who set it incorrectly. However, I think we should keep it where it currently is for backwards compatibility.
Without this middleware, tempfiles are not closed after the request, so they will not be cleaned up until garbage collection (this behavior comes from Tempfile using
I think Rack should continue to use this for the development and deployment environments for backwards compatibility.
I'm against removing it. Now, In terms of handlers:
|
|
Thanks for your feedback. Given that none of the middleware is strictly required, I think we should just remove all of them. The on the specific things that require lint, e.g. Thanks for explaining about Regarding the handlers. I'm on the fence. I think the bar should be that we have active tests to confirm it's working. In that regard, having a separate repo, e.g. I also took a look at how we could modify I think what we should aim to do is:
|
|
I still think we should keep the current middleware by default for backwards compatibility. If users don't want the current middleware, they can use a non development/deployment environment. The alternative is adding deprecation warnings for all usage of the development/deployment environment, which seems less palatable. I'm OK removing handlers without tests. We should add deprecation warnings to such handlers in 2.2, so that people that may be relying on them can write tests. There already exist tests for webrick, thin, and cgi. Parsing command line options in |
|
I don’t think anyone is using RACK_ENV=deployment thoughts. @tenderlove indicated a desire for removing the default middleware. I think it’s only being used by |
|
Regarding the default development/deployment middleware, if you and @tenderlove are in favor of removing them, I'm OK with that in 3.0 as long as it is documented. However, you should reconsider the idea that nobody is using |
|
I'm ok with removing the default middleware in v3.0.0. My overall "vision" is that Rack defines an interface that webservers and applications can use to communicate. It should have no opinions about what a default middleware is, as it's just defining an interface. Also, it means that theoretically apps that speak Rack could work with webservers that speak Rack and never depend on Rack itself. That said, I think it would be nice for Rack to provide things like:
Basically things that are nice to have but are never between the app and the webserver. Does that make sense as a direction? I think keeping the webrick handler around is fine as an example implementation for a webserver, but I do think we should rm the rest. |
|
I should have also mentioned: providing middleware seems fine too, but since my goal is for Rack to be "optional" in an app we shouldn't make defaults. |
|
Right - so if the server implements the SPEC, it can load apps any way it likes, it doesn't need to be |
|
I've been thinking about this over the past day and I came to some conclusions:
Firstly, let me state that I hate abbreviations, and As we can see, there are different environments and we use a single key to provide indirection w.r.t. which environment we want to select, often used to load a configuration file with the same name, e.g. There ARE some more things which are probably "global" state in rack which are passed as
(I understand you could probably argue either way for some of them). Regarding building a rack app, things like So, with that all in mind, I believe we should introduce a new interface for dealing with this. Something like: module Rack
class Environment < Hash
def self.build(env = ENV, multithread: false, multiprocess: false)
environment = self.new
environment['rack.env'] = env['RACK_ENV']&.to_sym
if app_env = env['APP_ENV']
environment['app.env'] = app_env.to_sym
else
environment['app.env'] = environment['rack.env']
end
environment['rack.multithread'] = multithread
environment['rack.multiprocess'] = multiprocess
end
def multithreaded?
self['rack.multithread']
end
def multiprocess?
self['rack.multiprocess']
end
def production?
self['app.env'] == :production
end
def development?
self['app.env'] == :development
end
def test?
self['app.env'] == :test
end
end
class Builder
def initialize(..., environment:)
@environment = environment
end
attr :environment
end
end
# config.ru
if environment.production?
use SendEmailErrorReports
end
if environment.multithreaded?
use Rack::Lock # oh no
endHowever, if at all possible, I'd like to think about whether |
|
Also, maybe We could also use it for servers to advertise features, e.g. etc This would be used by middleware to ensure the returned app can function (or not) on the given server. |
|
I'm against requiring Rack specific classes as part of the Rack API. As @tenderlove has mentioned, web servers and web applications need not require rack at all, as long as both conform to the rack API. I strongly feel the request env should remain a plain hash. The only place where Rack currently checks RACK_ENV is in the server and handlers. Some handlers use RACK_ENV to decide which address to bind to, and I don't think we want to change that as it can cause security issues. If we drop the default middleware for development and deployment, the server doesn't need to care about RACK_ENV. I'm against adding support for APP_ENV. I think Rack applications can respect it, but it should not effect Rack itself. |
I don't think it needs to be part of the SPEC. Because we don't include the definition of
As this is internal implementation detail, it can be reworked to avoid the use of a global variable IMHO, an would generally be a good idea.
It depends on how we want to expose this to the servers, and what kind of interface we should expect servers to conform to. Regarding |
I think that is fairly dangerous. If all users of Builder end up with I'm fine with servers (e.g. Falcon) using custom Hash subclasses for Note that the methods you are proposing for |
That's not my proposal though. It's an entirely new interface to expose the state of the world to the builder instance, rather than depending on global variables. Whatever you call it and however you implement it, I feel it should be certainly possible to do the following (the methods/arguments are just to illustrate the issue): Depending on global state, or some kind of per request state (in the case of multithread) just seems completely wrong to me. So, do you have any ideas how we can solve this? |
Sorry, I got confused since you are using the same keys that are currently used in the request environment per SPEC, and it's named
Is this actually enough of a problem to be worth solving? I currently don't think the benefit of adding the API is worth the cost. It recreates much of what web frameworks do, and I think it is something best left to web frameworks. That said, I'm not strongly against it, just not in favor. In terms of implementation, if it only supports the methods you are defining, there is no reason it should be a Hash subclass, a simple object with instance variables should be sufficient. Alternatively, it could be a Struct subclass. This is unless you want to support the following in if environment[:some_setting]
use SomeMiddleware
end |
|
Okay, I will spend some time thinking about what makes the most sense. |
I agree with this assessment. I find that ...It would be nice if this behavior was updated to reflect common community practices.
I agree with @tenderlove on that one. As both an app developer and a server maintainer, these defaults are sometimes surprising. I've had bug reports related to the default middleware producing changes to the response (and developers blaming my server).
Thank you @tenderlove for pointing out that the Rack interface might be used without the Rack gem. The iodine server supports this approach (a "rackless rack") and I used this approach before when I realized I wasn't using any Rack features. |
|
Summary of PumaAssessment: Options are an instance of a custom Iodinehttps://github.com/boazsegev/iodine/blob/71d4d6baf8feca7c78706497e220393d2cf6ab11/exe/iodine#L60-L78 Assessment: Options are an instance of Hash and are ignored. FalconAssessment: Standard UnicornAssessment: Completely custom parser and options handling. PassengerAssessment: Completely custom parser, ignores options. |
|
I wanted to know if any code is actually using Rack CodebaseAssessment: Not in use. Does anyone have any suggestion how we can check all of GitHub? |
|
I realised this is probably going to introduce breaking changes, because of the way vs I've also been thinking about this in the context of servers. Do we want |
|
I believe config.ru should continue to run at TOPLEVEL_BINDING. That is the expectation and I don't think any benefits of changing it are worth the loss of backwards compatibility. Note that if you provide a non- |
The benefits are:
In addition, I far prefer people using Although, I think this needs more experimentation. I don't disagree with you that it breaks some level of backwards compatibility. |
Just replacing builder = Rack::Builder.new(options)
binding = TOPLEVEL_BINDING.eval('builder.instance_eval { binding }')should work, no?
It's running in a copy of TOPLEVEL_BINDING, it doesn't add local variables to TOPLEVEL_BINDING.
Right, constants in config.ru will be constants of Object. Global variables are global no matter what, so there is no way to fix that except not using them. |
|
It's worth noting than running code in a copy of |
module Rack
class Builder
def initialize(**options)
@options = options
end
def self.load(script, **options)
builder = self.new(options)
binding = TOPLEVEL_BINDING.eval('builder.instance_eval { binding }')
binding.eval(script)
end
def debug
pp @options
end
end
end
Rack::Builder.load("self.debug", test: true)
# undefined local variable or method `builder' for main:Object (NameError) |
|
Ah yes, of course, but we can fix that: builder = self.new(options)
b = TOPLEVEL_BINDING.dup
b.local_variable_set :builder, builder
binding = b.eval('builder.instance_eval { binding }')That will have the side effect to also have that |
|
Haha, you are magician. |
|
Could also pass the builder via a lambda, very similar: builder = self.new(options)
binding = TOPLEVEL_BINDING.eval('-> builder { builder.instance_eval { binding } }').call(builder)
eval builder_script, binding, file
builder.to_appIt still leaks that one local variable though. |
RACK_ENVas a variable has many confusing use cases, and many of them don't completely align up.config.ru. Value include:test,:development, and:production. Frameworks are migrating to useAPP_ENVinstead.Rack::Serverit's used to control the default middleware. Values include:developmentand:deployment.I propose that we do the following:
1/ Change
Rack::Builderto take an optionalenvironmentparameter, which can be used during the construction of theconfig.ru.Then:
We can still feed RACK_ENV into this if required, but we decouple it and provide a non-global state interface for users.
2/ Provide some kind of "load an app" function under
Rack::Handler.The point is, if
Rack::Builderis not sufficient for generating the app, what is? Do we want to provide an interface for servers like Puma, Falcon, so they can essentially say: give me a rack middleware, for this environment, from this configuration file.In particular, I was looking at the middleware situation. Based on what @tenderlove told me, we can almost remove
environmentfrom the server, but I feel like there is value in automatically adding lint and so on during development.So, either we clearly define values for
environmentand what middleware they load, or we leave that up to downstream libraries, e.g.rack-testcan addRack::Lint.Some discussion here would be good. However, I may try throwing together a PR just so we have something actionable.
The text was updated successfully, but these errors were encountered: