Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass a request object to middleware rather than a hash #1879

Open
tenderlove opened this issue Apr 26, 2022 · 7 comments
Open

Pass a request object to middleware rather than a hash #1879

tenderlove opened this issue Apr 26, 2022 · 7 comments
Milestone

Comments

@tenderlove
Copy link
Member

tenderlove commented Apr 26, 2022

One problem I see in many middleware is that they "cast" the env hash to a Rack request object, do some manipulations, then pass then cast it back to a hash and pass it on.

For example:

def call(env)
  req = Rack::Request.new(env)

  # do stuff with req

  @app.call(req.env)
end

I would really like it if we could stop allocating request objects all the time, but since every middleware is passed a hash it's something that happens quite frequently.

I would like to do 2 things:

  1. Make a middleware that casts the env hash to a request object, then passes the request object to the next middleware
  2. Make the request object quack enough like a hash that existing legacy middleware can use the request object as if it was the env hash

Step 1 could be as simple as this:

class CastsToRequest
  def initialize(app)
    @app = app
  end
  def call(env); @app.call(Rack::Request.new(env)); end
end

If some middleware want to take advantage of this "pre allocated" request object, then they would require that CastsToRequest is in the middleware stack above them. Those middleware would no longer have to cast back and forth between hash and request object.

If the Rack::Request object quacks enough like a hash, then users could add legacy middleware without changing anything, though they could be upgraded to take advantage of the request object.

I think this idea could work because it should be OK for any middleware to say "yes I work with any Rack compatible web server, but I must be run after the CastsToRequest middleware. I think this would also unlock the ability for web servers to start passing their own request objects to middleware (as long as they quack the same as Rack::Request).

@jeremyevans
Copy link
Contributor

As I mentioned in #1878 (comment), I'm against this proposal. Completely breaking backwards compatibility at this stage in rack's lifecycle makes is a very bad trade off, IMO.

If given a choice between a request object and an h2 style env, I think the request object approach is better, as it offers advantages and doesn't just rearrange the deck chairs. That being said, if we want to use a request object, we should specify (in SPEC) each method the request object should respond to, and how each method should operate. We should not specify that the request object should operate like Rack::Request. For one, the Rack::Request API footprint is gigantic. Second, one of Rack's basic principals is that you do not need to require rack the library in order to have a rack compliant server or application.

@tenderlove
Copy link
Member Author

@jeremyevans what part of this proposal are you against? The contract with the webserver doesn't change, it just lets downstream rack applications avoid allocating more request objects (should they want to opt-in, and they must opt-in by having the CastsToRequest app upstream)

@jeremyevans
Copy link
Contributor

@tenderlove I don't think it's a good idea to change the object passed to middleware/applications. Part of your comment makes this appear to be opt-in, which is not nearly as bad as having it on by default, but I still don't think it's a good idea. You would have to complicate all middleware that ship with rack to deal with both env hash and Rack::Request, and all external middleware and all applications would have to deal with both types or using CastsToRequest middleware would break things. This is a huge burden on the entire rack ecosystem.

Additionally, The contract with the webserver doesn't change in some way conflicts with I think this would also unlock the ability for web servers to start passing their own request objects to middleware. You are at least giving the webserver the option to completely break the current contract. If a popular webserver starts doing that, de facto that becomes something that all rack middleware/applications have to support. Again, it would put a huge burden on the entire rack ecosystem.

If the goal is to avoid allocations, my recommendation would be to add the following to current middleware that ship with Rack:

  # change
  request = Rack::Request.new(env)
  # to:
  request = env['rack.request'] ||= Rack::Request.new(env)

What are your thoughts on that approach?

@tenderlove
Copy link
Member Author

You would have to complicate all middleware that ship with rack to deal with both env hash and Rack::Request, and all external middleware and all applications would have to deal with both types or using CastsToRequest middleware would break things. This is a huge burden on the entire rack ecosystem.

Sorry I don't follow this. A middleware would have to say "I only work when CastsToRequest is upstream". That means there would be no conditionals, and is "opt in". If the request object quacks enough like a hash, then legacy middleware wouldn't need to care whether it's a Rack request or a hash. Can you be more specific on how this would be a burden to the ecosystem?

What are your thoughts on that approach?

We've tried this exact approach in Rails before and ended up removing it. Though I have to admit I don't remember why. Regardless, I would prefer not to litter every middleware with a lazy allocator and I would also prefer not to rely on the good graces of all previous middleware to pass the same env hash all the way down the stack.

@jeremyevans
Copy link
Contributor

You would have to complicate all middleware that ship with rack to deal with both env hash and Rack::Request, and all external middleware and all applications would have to deal with both types or using CastsToRequest middleware would break things. This is a huge burden on the entire rack ecosystem.

Sorry I don't follow this. A middleware would have to say "I only work when CastsToRequest is upstream". That means there would be no conditionals, and is "opt in". If the request object quacks enough like a hash, then legacy middleware wouldn't need to care whether it's a Rack request or a hash. Can you be more specific on how this would be a burden to the ecosystem?

I guess you are working under the assumption that you can get a request object that operates enough like an env hash that middleware and applications will not need changes. I think that's a questionable assumption. Quacks enough like a hash is a fairly gray area. Can I pass it to a method implemented in C that accepts T_HASH? Does it implement all of Hash's API?

Unless you are able to make this transparent, so that a request object can be used anywhere an env hash is currently used, I think this will place a burden on the ecosystem. It either increases complexity for all middleware/applications to support both, or it bifurcates the ecosystem and middleware/applications support one and not the other.

Additionally, is it really opt-in if webservers are allowed to use a request object instead of an env hash? Or do you expect that you can opt-in at the webserver to get that behavior, and it won't be the default?

@jeremyevans
Copy link
Contributor

If the issue is middleware that ship with Rack, as an alternative to this, I volunteer to modify all middleware that ship with rack to not rely on Rack::Request.

@HoneyryderChuck
Copy link

Been slowly catching up to the several threads about the topic, and it seems that there are two valid perspectives: on one side, the protocol needs to evolve, while on the other, breaking code relying on the current contract is unacceptable. So why not just create a new entrypoint, and thereby, a new protocol, that could reuse most of the existing boilerplace?

I'd propose smth like this:

def call_req(request)
  # do stuff with req

 # get a response object
  response = @app.call_req(request)
  return response
end

This could provide new ground for speccing smth which the current rack protocol is limited for, i.e. partial responses, streaming, etc, while allowing for removing the current reliance on the CGI protocol headers.

The rack library could then provide a compatibility layer to allow for "new protocol" middleware to drop-down to "old protocol" when needed. Consider these middlewares:

class NewMiddleware
  def call_req(req)
    @app.call_req(req)
  end
end

class OldMiddleware
  def call(env)
     @app.call(env)
  end
end

class DualMiddleware
  def call_req(req)
     @app.call_req(req)
  end

  def call(env)
     @app.call(env)
  end
end

The rack library would be, in the rack app middleware stack chaining, be responsible for injecting "shim handlers" for the unhandled protocol versions:

class NewMiddleware
  # rack would inject: attr_reader :request_class
  def call_req(req)
     @app.call_req(req)
  end

  # rack would inject
  def call(env)
    call_req(@request_class.from_env(env))
end

class OldMiddleware
  def call(env)
    @app.call(env)
  end

  # rack would inject
  def call_req(request)
    rack_array = call(request.to_env)

    request.make_response(rack_array)
  end
end

If we skip bikeshedding on the names above, the benefit would be that old middlewares would play well with new middlewares and vice-versa, in any order, with negligible impact (1 extra method call in the stack). Middlewares could be adapted for both versions. Frameworks could provide entrypoints for both versions and adapt accordingly. Application servers would be in charge with passing down request and response interfaces complying with the new protocol.

I also agree with @jeremyevans that Rack::Request has too big method footprint to deem it the standard interface to implement. But the point is, in order for new interfaces to be speccable, rack needs a new entrypoint beyond call which allows current code to execute, and provide a migration path to a new protocol which can reuse most of the old middlewares around, while allowing to create new ones which the current protocol can't handle.

(I've tried, and ultimately gave up on, years ago, to build an alternative to rack).

@ioquatix ioquatix added this to the v3.1.0 milestone May 3, 2022
userlea referenced this issue May 8, 2022
…test-sprint

Add rack-session gem when testing rack-attack
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants