Review Board 1.5.4

contextFuncBuilder doesn't capture all request state.

Updated 2 years, 2 months ago

Marius Danciu Reviewers
191 dchenbecker, dpp, marius, charles, joni, atsuhiko, jorge, naftoli, kris, indrajit, alexb
None LiftWeb-archive 
This fix also improves the fix issue 56 as snippets processed asynchronously would benefit on the request information.


Br's,
Marius  
Ad hoc. Request params are captured quite nicely.
Ship it!
David Pollak
Posted 2 years, 2 months ago (November 15th, 2009, 11:30 a.m.)
This may lead to doubling initialized sessions and I'm not sure the about the smarts of doing that.

Let's see if this actually breaks anything.
  1. In passing... how about renaming S.init(request, session) to S.doWith(request, session) to follow similar conventions on Req and LiftSession?  
(I can make a patch for the review board)
  2. No changing method names.  This stuff is used everywhere and anything that's not private to Lift not subject to "this is better so let's change it now".

Further, review board is *not* the right place to discuss this stuff.
  3. +1 David.

I'm not sure about double initialization of sessions since both Req and LiftSession are already existent. The init just set the proper ThreadGlobals. The only thing I'm a little worried is that _init does initialization of RequestVarHandler which was previously captured. But I will do a bit more testing on this. I could add an private[http] light init that would not include the RequestVarHandler initialization and see how it behaves. Will get back soon on this.
Marius Danciu
Posted 2 years, 2 months ago (November 18th, 2009, 10:48 p.m.)
I need an alternative fix for capturing request parameters as S.init inside contextFuncBuilder proved to induce side effects.
Marius Danciu
Posted 2 years, 2 months ago (November 18th, 2009, 10:48 p.m.)
I need an alternative fix for capturing request parameters as S.init inside contextFuncBuilder proved to induce side effects.
  1. DPP really needs to give you feedback on this dude - perhaps give him a quick prompt?
  2. I have an offline discussion with David so don't think this is on hold :) ... I do have a fix on my box that captures the context correctly but there are some tricky things to be address about when the shutdown piggy backed comet in conjunction with comet lifespan and LiftRules.LiftRules.noCometSessionPage redirects.

Br's,
Marius
Review request changed
Updated 2 years, 2 months ago (November 30th, 2009, 11:37 p.m.)
Here are the updates for request context capturing and lazy-load comet transparent termination.

Br's,
Marius
Ship it!
Ross Mellgren
Posted 2 years, 2 months ago (December 2nd, 2009, 6:45 a.m.)
I can't say that I fully understand what this code is for so I wasn't able to reason too much about whether the change has unintended side effects and so on, but I did a quick syntax review and had a couple small questions.

I think they're minor though.
takes attrs, but does not apparently use it?
  1. Good catch. attrs arg. needs to be put in _attrs.doWith... I'll update that.
why make copies of the entire list spine for _cookies, as opposed to using the donor req's cookies directly? It doesn't appear that you can change the cookies.
  1. Because that HTTPRequest is used on a different thread and keeping a strong reference to the servlet's HttpServletRequest after the response is sent is not a good thing. This is why it is called OfflineRequestSnapshot. It takes the request state without capturing the actual request reference managed by container.
  2. okay then 
    def session: HTTPSession = req.session
should become val so that it stops closing on req, right?
same as previous comment
  1. Same as previous answer.
same as previous comment
  1. Same as previous answer.
Ship it!
Ross Mellgren
Posted 2 years, 2 months ago (December 2nd, 2009, 6:46 a.m.)
I can't say that I fully understand what this code is for so I wasn't able to reason too much about whether the change has unintended side effects and so on, but I did a quick syntax review and had a couple small questions.

I think they're minor though.
Ship it!
David Pollak
Posted 2 years, 2 months ago (December 2nd, 2009, 8:29 a.m.)
Rockin awesome stuff
Ship it!
David Pollak
Posted 2 years, 2 months ago (December 2nd, 2009, 8:29 a.m.)
Rockin awesome stuff