Stateful Sessions and LoanWrappers

[David Whittaker]

I've been looking into an issue with Squeryl-Record's Crudify trait and while it doesn't seem to be directly related, I'm seeing LoanWrapper behavior that I did not expect.  In particular, accessing the /list page is resulting in 3 separate calls to the LoanWrapper that creates a transaction.  It happens once when LiftServlet calls S.statelessInit(req) prior to testing whether the request is stateful or not, once when LiftServlet.getLiftSession is called prior to initializing a stateful session, and then once again when the stateful session is actually initialized for the request.  Is this by design?  For my purposes, grabbing a connection from a connection pool and then returning it before it's needed again doesn't seem like it would be a real performance issue, but I wonder if this might be an issue for people using LoanWrappers for other purposes.  The ScalaDoc does state:

This trait defines the principle contract for function objects that wrap the processing of HTTP requests by Lift while utilizing the preestablished request-local scope.

Which I had interpreted to mean one execution per-request.

[Jeppe Nejsum Madsen]

I ran into the same issue a while back and dpp stated it was, if not
entirely by design, then due to the way stateless/stateful handling
turned out.

I also think this is less than optimal for the same reasons you state
and I was planning to look into this at some point but haven't had the
time.

/Jeppe

[David Pollak]

I could move the loan wrapper code to the same place that the request vars are done... the issue, though, is that sometimes the LoanWrappers depend on state and it'll break everything that depends on state for computing the loan wrapper.

Alternatively, I could create an outer loan wrapper that is wrapped at the same place that RequestVars are and that could be migrated to if you need a single LoanWrapper invocation per request.

--
Lift, the simply functional web framework: http://liftweb.net
Code: http://github.com/lift
Discussion: http://groups.google.com/group/liftweb
Stuck? Help us help you: https://www.assembla.com/wiki/show/liftweb/Posting_example_code

--

Visi.Pro, Cloud Computing for the Rest of Us http://visi.pro

Lift, the simply functional web framework http://liftweb.net

Follow me: http://twitter.com/dpp
Blog: http://goodstuff.im

[Jeppe Nejsum Madsen]

On Mon, Dec 12, 2011 at 7:55 PM, David Pollak
<feeder.of...@gmail.com> wrote:
> I could move the loan wrapper code to the same place that the request vars
> are done... the issue, though, is that sometimes the LoanWrappers depend on
> state and it'll break everything that depends on state for computing the
> loan wrapper.
>
> Alternatively, I could create an outer loan wrapper that is wrapped at the
> same place that RequestVars are and that could be migrated to if you need a
> single LoanWrapper invocation per request.

I think the ideal solution is for LoanWrappers to have a single
invocation per request and be able to check for S.statefulRequest_? to
determine if state is available. But I guess this was difficult with
the current setup (I haven't looked at the code yet :-)

How would LoanWrappers that depend on state behave in a stateless
request? Perhaps we should have both addAroundStateful &
addAroundStateless....

/Jeppe

[David Pollak]

I'd rather create a new method and deprecate the old methods.  That'll cause the least migration pain.

 

/Jeppe

--
Lift, the simply functional web framework: http://liftweb.net
Code: http://github.com/lift
Discussion: http://groups.google.com/group/liftweb
Stuck? Help us help you: https://www.assembla.com/wiki/show/liftweb/Posting_example_code

[David Whittaker]

David,

Can you point me at the spot where the RequestVars are initialized so I can take a look?  All of the LoanWrapper invocations seem to happen within this else: https://github.com/lift/framework/blob/master/web/webkit/src/main/scala/net/liftweb/http/LiftServlet.scala#L258 so it seems like wrapping it with a single invocation might work.  If changes are going to be made, I'd like to see is the LoanWrapper invocation happen around Loc rewrites as well so that you can set up your transaction in your LoanWrapper and not have to explicitly start one when you look up objects, etc, to see if a rewrite rule matches.

[David Pollak]

It's in ServletFilterProvider.scala:

  def doFilter(req: ServletRequest, res: ServletResponse, chain: FilterChain) = {

    if (LiftRules.ending) chain.doFilter(req, res)

    else {

      LiftRules.reqCnt.incrementAndGet()

      try {

        TransientRequestVarHandler(Empty,

                                   RequestVarHandler(Empty,

                                                     (req, res) match {

              case (httpReq: HttpServletRequest, httpRes: HttpServletResponse) =>

                val httpRequest = new HTTPRequestServlet(httpReq, this)

                val httpResponse = new HTTPResponseServlet(httpRes)

                service(httpRequest, httpResponse) {

                  chain.doFilter(req, res)

                }

              case _ => chain.doFilter(req, res)

            }))

      } finally {LiftRules.reqCnt.decrementAndGet()}

    }

  }

[David Whittaker]

Ok, we can't get much earlier in the request than the Servlet Filter so that makes sense to me.  If we wanted to go the add/deprecate route maybe an S.addAroundStateless (S.addAroundEarly?) that happens at that point and then an S.addAroundStateful that occurs when the stateful session is initialized?  If something along those lines gets implemented I wonder if it might makes sense to re-think the name LoanWrapper as well.  It may just be me, but on first look I had no idea what that meant.  I think that something like RequestWrapper would be easier for newbies to grasp and similar enough to not throw experienced Lifters off too much. 

[David Pollak]

On Fri, Dec 23, 2011 at 2:29 PM, David Whittaker <da...@iradix.com> wrote:

Ok, we can't get much earlier in the request than the Servlet Filter so that makes sense to me.  If we wanted to go the add/deprecate route maybe an S.addAroundStateless (S.addAroundEarly?) that happens at that point and then an S.addAroundStateful that occurs when the stateful session is initialized?  If something along those lines gets implemented I wonder if it might makes sense to re-think the name LoanWrapper as well.  It may just be me, but on first look I had no idea what that meant.  I think that something like RequestWrapper would be easier for newbies to grasp and similar enough to not throw experienced Lifters off too much. 

The "Loan" pattern was an early Scala pattern: https://wiki.scala-lang.org/display/SYGN/Loan  I'd prefer to keep the name, if for no other reason than to pay homage to Scala's roots.

I also think the addAllAround should be on LiftRules, not S, because it's no longer got anything to do with S.

[David Whittaker]

On Fri, Dec 23, 2011 at 2:29 PM, David Whittaker <da...@iradix.com> wrote:

Ok, we can't get much earlier in the request than the Servlet Filter so that makes sense to me.  If we wanted to go the add/deprecate route maybe an S.addAroundStateless (S.addAroundEarly?) that happens at that point and then an S.addAroundStateful that occurs when the stateful session is initialized?  If something along those lines gets implemented I wonder if it might makes sense to re-think the name LoanWrapper as well.  It may just be me, but on first look I had no idea what that meant.  I think that something like RequestWrapper would be easier for newbies to grasp and similar enough to not throw experienced Lifters off too much. 

The "Loan" pattern was an early Scala pattern: https://wiki.scala-lang.org/display/SYGN/Loan  I'd prefer to keep the name, if for no other reason than to pay homage to Scala's roots.

Ah, I'd never seen that before.  Ok, I've got no argument there.

I also think the addAllAround should be on LiftRules, not S, because it's no longer got anything to do with S.

Good point.  Does addAllAround sound right to you though?  It feels to me like a method with that name should accept a list. Maybe S.aroundService / LiftRules.aroundService?  I think aroundRequest would be the most self explanatory but the request & response are wrapped I suppose it could be confusing.

[David Pollak]

On Tue, Dec 27, 2011 at 2:07 PM, David Whittaker <da...@iradix.com> wrote:

On Fri, Dec 23, 2011 at 2:29 PM, David Whittaker <da...@iradix.com> wrote:

Ok, we can't get much earlier in the request than the Servlet Filter so that makes sense to me.  If we wanted to go the add/deprecate route maybe an S.addAroundStateless (S.addAroundEarly?) that happens at that point and then an S.addAroundStateful that occurs when the stateful session is initialized?  If something along those lines gets implemented I wonder if it might makes sense to re-think the name LoanWrapper as well.  It may just be me, but on first look I had no idea what that meant.  I think that something like RequestWrapper would be easier for newbies to grasp and similar enough to not throw experienced Lifters off too much. 

The "Loan" pattern was an early Scala pattern: https://wiki.scala-lang.org/display/SYGN/Loan  I'd prefer to keep the name, if for no other reason than to pay homage to Scala's roots.

Ah, I'd never seen that before.  Ok, I've got no argument there.

I also think the addAllAround should be on LiftRules, not S, because it's no longer got anything to do with S.

Good point.  Does addAllAround sound right to you though?

You do the hokey pokey and you LiftRules.addAllAround sounds find to me. ;-)

 

 It feels to me like a method with that name should accept a list.

How about () => LoanWrapperOrListOfLoanWrapper

That way you can have some nice implicits to do either a single loan wrapper or a list of loan wrappers?

[David Whittaker]

How about () => LoanWrapperOrListOfLoanWrapper
That way you can have some nice implicits to do either a single loan wrapper or a list of loan wrappers?

I know the ability to pass a List exists in the current implementation, but is it commonly used?  In my own experience there is generally one LoanWrapper that sets up whatever state you need for the request.  I just wonder if the extra complexity is worth the confusion it might cause for new users who will be trying to figure out what that type is and how to create one, or what they need to have imported to get the conversion to happen.

[David Pollak]

On Wed, Dec 28, 2011 at 9:07 AM, David Whittaker <da...@iradix.com> wrote:

How about () => LoanWrapperOrListOfLoanWrapper
That way you can have some nice implicits to do either a single loan wrapper or a list of loan wrappers?

I know the ability to pass a List exists in the current implementation, but is it commonly used?

Fair enough.  Let's just do the single loan wrappers.

[David Whittaker]

Ok, should I create a ticket?

[David Pollak]

On Thu, Dec 29, 2011 at 1:39 PM, David Whittaker <da...@iradix.com> wrote:

Ok, should I create a ticket?

Yes, please

[David Whittaker]

Ok, http://www.assembla.com/spaces/liftweb/tickets/1177-single-loanwrapper-invocation-per-request created.  David, I took the liberty of assigning it to you since you mentioned earlier that you would be willing to implement.  Hopefully that's ok.

Thanks,

Dave

[David Pollak]

On Tue, Jan 3, 2012 at 12:41 PM, David Whittaker <da...@iradix.com> wrote:

Ok, http://www.assembla.com/spaces/liftweb/tickets/1177-single-loanwrapper-invocation-per-request created.  David, I took the liberty of assigning it to you since you mentioned earlier that you would be willing to implement.  Hopefully that's ok.

Yep.  Monday, Jan 8, I'm going to be plowing, PLOWING, ***PLOWING*** through tickets!!!  Wooooo Hooo!!! can't wait!

[Naftoli Gugenheim]

Good point.  Does addAllAround sound right to you though?  It feels to me like a method with that name should accept a list. Maybe S.aroundService / LiftRules.aroundService?  I think aroundRequest would be the most self explanatory but the request & response are wrapped I suppose it could be confusing.

How about aroundAll rather than allAround (all around sounds like sprinkle it everywhere).

Also if it's in LiftRules, perhaps it should not be an imperative-sounding method like "add around" but more in line with other "rules," such as dispatch (you don't call addDispatch, you just configure dispatch).

[David Pollak]

Great points!

 

--
Lift, the simply functional web framework: http://liftweb.net
Code: http://github.com/lift
Discussion: http://groups.google.com/group/liftweb
Stuck? Help us help you: https://www.assembla.com/wiki/show/liftweb/Posting_example_code