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

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

Closed
wants to merge 4 commits into from

Conversation

ioquatix
Copy link
Member

@ioquatix ioquatix commented Nov 13, 2020

There are several goals for this PR.

  • Remove the per-request rack.multithread/rack.multiprocess/rack.run_once that usually come too late to be of any use.
  • Add some build time context which can be accessed from the Rack::Builder.
  • Ensure the interface is generic and robust enough to encourage modular middleware design.

Rack::Lock is updated for the new interface.

It also allows 0 or more middleware instances to sit within Middleware.rackup. This can be used for applications which need to internally construct multiple middleware such as Rails, taking into consideration server features.

I also experimented with such a design, where .rackup would be similar to .call in https://github.com/socketry/utopia-project/blob/master/lib/utopia/project.rb#L47-L87

These variables generally come too late to be useful. Removed `Rack::Lock` which depends on these variables.
@ioquatix
Copy link
Member Author

After discussing this with @jeremyevans, I want to share some of the main points:

@jeremyevans is concerned about the issue of encapsulation, and exposing middleware to the builder instance. I actually agree, but my design has some trade-offs to avoid introducing new classes/specifications.

Firstly, his counter proposal was something like this:

class Rack::Lock
  def self.rackup(context)
    if context.multithreaded?
       return self # class itself
    end
  end
end

The middleware class comes back to the builder which calls Rack::Lock.new(...).

However, there are some problems with this.

Consider the following:

class OtherMiddleware
end

class MyMiddleware
  def self.rackup(context)
    OtherMiddleware
  end
end

The builder instance will invoke OtherMiddleware.new after calling MyMiddleware.rackup. However, consider later that:

module OtherMiddleware
  def self.rackup(context)
    return # ...
  end
end

This will no longer work because MyMiddleware.rackup returns OtherMiddleware which also expects to be invoked using rackup.

This issue is very tricky to solve correctly.

The only way to make this work is to recursively call rackup in Rack::Builder until nil or a class without rackup is found. The other option is simply to fail in this case.

@ioquatix ioquatix changed the title Remove execution variables Remove per-request "features" and build a consistent way to expose this to middleware. Nov 14, 2020
@ioquatix ioquatix changed the title Remove per-request "features" and build a consistent way to expose this to middleware. Remove per-request "features" and provide a consistent way to expose this to middleware. Nov 14, 2020
@ioquatix ioquatix changed the title Remove per-request "features" and provide a consistent way to expose this to middleware. Remove per-request "server implementation details" and provide a consistent way to expose this to middleware during build time. Nov 14, 2020
@jeremyevans
Copy link
Contributor

After thinking about this a lot overnight, I came up with a slightly different API than I discussed earlier with @ioquatix . I added a pull request for my alternative approach in #1720. It actually ends up a lot like this approach, with the main difference being the first argument to the middleware rackup method is a configuration object instead of a builder object. This approach handles the recursion issue he mentions, and doesn't require the creation of Wrapper classes in middleware, so it should be less prone to unbounded recursion.

The question of which approach to use boils down to how much we want to expose to middleware, and whether we want to encourage the use of builder by middleware. My approach in #1720 limits the information to just what middleware should need for configuration. This PR does not add any features that #1720 lacks, since any middleware authors who actually want a full builder can simply create the builder manually in their middleware rackup method.

@ioquatix also expressed concern about the complexity of my approach. The approach in #1720 is shorter than this PR by a few lines, and I think it is either equivalent or a tad simpler.

#1720 includes the first two commits from this PR. I do not like the idea of a default app, so #1720 does not include that. Without a default app, a missing app results in an error. With a default app, a missing app results in a 404 page, and that's going to be confusing for users (why is my app always returning 404 regardless of the changes I'm making to it?)

In terms of the implementation of this PR, there are two problems, both easily fixable:

  • middleware block is ignored if middleware responds to rackup
  • keywords arguments are not handled correctly on Ruby 2.7+ by Builder#rackup (should use ruby2_keywords if available)

It would be great to get feedback from other users and committers on which approach they would prefer.

@ioquatix
Copy link
Member Author

Closing in favour of @jeremyevans's implementation.

@ioquatix ioquatix closed this Jan 23, 2022
@ioquatix ioquatix deleted the remove-execution-variables branch April 26, 2022 02:42
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

Successfully merging this pull request may close these issues.

None yet

2 participants