Lazy-loaded and memoized processing

[Yehuda Katz]

Hey guys. I just wanted to post about a problem I've been working through and a potential solution to get some feedback. Effectively, there's a bunch of related code that handles various processing of the Rack environment. However, much of this processing is triggered by requests by the user, and can be expensive. The current recommended approach is to create a middleware that processes the rack environment as needed, and stuff the results back into the environment hash. This works fine for cases where you know the processing will be needed, but not as well for cases where the processing is to be used optionally by a user downstream in the middleware chain. Before rack, the approach both Rails and Merb used was to do that processing in a Request object that could process the environment only when a specific accessor was called. Some examples include "method", a 10-line method in Merb, various parameter parsing (which can be quite expensive), remote_ip (another function that is longer than it seems like is reasonable), and it goes on and on. What is desirable is for the appropriate processing to be done when required, and only once. Whether or not the user is in the Rails/Merb application or Rack middleware should not affect this (the user should have access to any required processing). Unfortunately, since the Rack environment is required to be a "real" Hash, it's not possible to modify the environment to support lazy-loaded and memoized processing. One option we *considered* was a middleware that did something like env["rack.request_obj"] = Rack::Request.new(env). This would provide the benefits of a logical object for the request without breaking Rack conventions. However, after emailing Chris to explain this problem, he suggested an even better (to my eyes) solution: > I think I have a better idea for this... we could make > Rack::Request.new cache the request object in the env itself(!) and > just return it when it was created and calculated already. This would > be backward compatible, efficient, and not expose any details on the > implementation. I love this idea and would love to get some feedback on it. Thanks a ton,

--
Yehuda Katz
Developer | Engine Yard
(ph) 718.877.1325

[Matt Todd]

Effectively...

def Rack::Request.from(env)

  unless env['rack.request']

    env['rack.request'] = Rack::Request.new(env)

  end

  env['rack.request']

end

Hmm?

Matt

--
Matt Todd
Highgroove Studios
www.highgroove.com
cell: 404-314-2612
blog: maraby.org

Scout - Web Monitoring and Reporting Software
www.scoutapp.com

[Yehuda Katz]

Yep!

This allows any middleware to use Rack::Request throughout the chain without having to worry about the overhead of continually setting up a new object and having to reprocess.

w00t!

-- Yehuda

[Yehuda Katz]

I think Chris specifically wanted to override new, as opposed to making a new builder, for BC-related reasons.

-- Yehuda

[Christian Neukirchen]

"Yehuda Katz" <wyc...@gmail.com> writes:

> I think Chris specifically wanted to override new, as opposed to making a new
> builder, for BC-related reasons.
>
> -- Yehuda

Yep. Also we can move the Request-internal caches (form_vars etc) to
instance variables then.

--
Christian Neukirchen <chneuk...@gmail.com> http://chneukirchen.org

[Joshua Peek]

Tiny detail, override new for better backwords compat and a nicer API I think.

--
Joshua Peek

[Matt Todd]

Yeah, just ignored that for the sake of simplicity.

class Rack::Request

  def self.new(env)

    unless env['r']

      env['r'] = super(env)

    end

    env['r']

  end

  def initialize(env); @env = env; end

end

I'll work on a patch tonight unless someone beats me to it.

Matt

[Matt Todd]

Implementing now, tests look fine except for the MockRequest test...

test_spec {Rack::MockResponse} 002 [should provide access to the HTTP headers](Rack::MockResponse) [./test/spec_rack_mock.rb:130]:

<381>

with id <763> expected to be equal? to

<444>

with id <889>.

The problem is that it's explicitly testing for content length of 381 and it's now 444 with the new rack.request reference.

There are two solutions... one, change the test to test against 444 content-length, obviously.

But I don't think this addresses the real problem: he app is calling env.to_yaml and setting it as the body for the mock response.

The other solution could be to define a way to remove rack.request from the collection of headers before calling to_yaml on that hash, but I don't know if this is a legitimate issue that requires this exceptional behavior.

Looking for opinions; share any you have please.

Matt

[Matt Todd]

Correction, the to_yaml is called on the environment variable directly on not on the Rack::Request object so it's not to be helped that the content length is slightly longer.

I've updated the test with the new length.

Pull request sent via GitHub, refers to the following commits:

http://github.com/mtodd/rack/

eefbed89c4ece749e889132012d0f67cd87926a8

3873269bea8d29e3ae4f1f46eb11822f9eae795f

552f7b0718ee8cd79c185cd72413690f0da72402

This includes:

* Rack::Request memoization in environment passed in implementation

* Adjustment to MockRequest content-length expected (now 444)

* Spec to determine that the Rack::Request is memoized for the same environment

Some concerns/questions:

* I didn't take any account into marshalling/loading of the request... should this be a concern at all?

* I did not add tests (though I did test it) for unmemoizing the rack.request value... should I add this in?

Matt

[Joshua Peek]

Looks good

--
Joshua Peek

[Scytrin dai Kinthra]

I like. Saves silly overhead.

--
stadik.net

[Christian Neukirchen]

"Matt Todd" <chio...@gmail.com> writes:

> Correction, the to_yaml is called on the environment variable directly on not
> on the Rack::Request object so it's not to be helped that the content length is
> slightly longer.
>
> I've updated the test with the new length.

Obviously that test "tests too much". Your fix is okay.

Applied.

[Christoffer Sawicki]

On Thu, Dec 25, 2008 at 12:57 AM, Matt Todd <chio...@gmail.com> wrote:
> unless env['r']
> env['r'] = super(env)
> end
> env['r']

Isn't env['r'] ||= super(env) more succinct?

Regards,
Christoffer Sawicki

[Matt Todd]

Yeah, makes much more sense.

I believe (though I don't remember the exact reason) I started that way because I wanted to expand the test to be more than just existence but also of class, et al. That, however, wasn't necessary, or, at least, not yet anyways.

Matt