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

Req and LiftSession - aware Futures / LAFutures #1813

Merged
merged 9 commits into from Dec 22, 2016

Conversation

Projects
None yet
3 participants
@pdyraga

pdyraga commented Oct 8, 2016

Session-aware futures seems to be a recurring pain point in many projects. In the one I work, we often use Futures to lazy-load some part of a page, e.g. retrieve a list of users from a database and then apply Lift bindings on them.

In Lift3 Future template binding is straightforward but one thing is still missing: you can’t access session from them which is not so crazy as it might sound.

Continuing with users from database example, say that we retrieved a Future[Seq] of users and now we want to .map it to produce bindings that will be later bound to element. In that bindings, we’d like to use S.? to render localised user type text. Currently it’s not possible, because session will not be available in thread doing .map call.

We already have a PR addressing this issue #1730 but there are few things missing there in my opinion:

  • lack of ReqVar access. I think that we should offer the same access scope as LazyLoad that we already have in Lift
  • lack of session access in Future / LAFuture body
  • We shouldn’t use CommonLoanWrapper for session context wrapping, because CommonLoanWrapper acts lazily which may lead to session wrapping to be done in separate thread which is too late. We need to enrich function with session access eagerly.

Sample usage of feature implemented in this PR can be:

<div id="main" data-lift="HelloWorld.render">
  Future: <span id="scala-future">Time goes here</span><br/>
  LAFuture: <span id="lift-future">Time goes here</span>
</div>
class HelloWorld {

  def render = {
    "#lift-future *" #> LAFutureWithSession.withCurrentSession {
      Thread.sleep(3000);
      S ? ("general-futureCompleted", date)
    }.map(s => s"$s in request from = ${S.request.map(_.userAgent).openOrThrowException("No request!")}") &
    "#scala-future *" #> FutureWithSession.withCurrentSession {
      Thread.sleep(1000);
      S ? ("general-futureCompleted", date)
    }.map(s => s"$s in request from = ${S.request.map(_.userAgent).openOrThrowException("No request!")}")
  }

  private def date: String = {
    new SimpleDateFormat("yyyy/MM/dd HH:mm:ss").format(new Date())
  }
}

Mailing list reference: https://groups.google.com/forum/#!topic/liftweb/qXDBuSTfWnw

Request/session - aware futures
Lift 3 allows to bind Future / LAFuture instance to template elements easily. Still, when Future gets transformed with one of its map/flatMap/etc methods or when Future body gets executed we don't have an access to current Req or LiftSession. This issue seems to be recurring between various projects using Lift framework. This commit brings request and session-aware futures. It allows to create such a type of LAFuture or decorate Scala's Future instance so that Req and LifSession parameters can be accessed from it.
@pdyraga

This comment has been minimized.

pdyraga commented Oct 8, 2016

@farmdawgnation

This comment has been minimized.

Member

farmdawgnation commented Oct 8, 2016

Looks like this spec is failing, which seems pretty important :)

net.liftweb.http.LAFutureWithSessionSpec

@farmdawgnation

Finished my first (rough) pass. Looking cool, but there are some changes I'm requesting here. :)

@@ -105,16 +101,18 @@ class LAFuture[T](val scheduler: LAScheduler) {
* @return a Future that represents the function applied to the value of the future
*/
def map[A](f: T => A): LAFuture[A] = {
val ret = new LAFuture[A](scheduler)
onComplete(v => ret.complete(v.flatMap(n => Box.tryo(f(n)))))
val ret = new LAFuture[A](scheduler, context)

This comment has been minimized.

@farmdawgnation

farmdawgnation Oct 8, 2016

Member

While we're in here can we give ret and f a better name? 👼

This comment has been minimized.

@pdyraga

pdyraga Oct 8, 2016

I can rename ret to result (any other suggestions appreciated) but have no idea for a better name for f. I looked at Box and Option and they also use just f as param name. 🤔

ret
}
def flatMap[A](f: T => LAFuture[A]): LAFuture[A] = {
val ret = new LAFuture[A](scheduler)
val ret = new LAFuture[A](scheduler, context)

This comment has been minimized.

@farmdawgnation

farmdawgnation Oct 8, 2016

Member

Ditto here.

* It may choose to execute or not execute that functionality, but should not interpret
* or change the returned value; instead, it should perform orthogonal actions that
* need to occur around the given functionality. Typical example is setting up DB
* transaction.

This comment has been minimized.

@farmdawgnation

farmdawgnation Oct 8, 2016

Member

I'm finding this a bit hard to follow, but I'm not entirely sure why... it's entirely possible that I haven't had enough caffeine yet, but I'm flagging this anyway. ☕️

This comment has been minimized.

@pdyraga

pdyraga Oct 10, 2016

I'm not a native speaker, so any improvement ideas for this scaladoc are very appreciated :)

As an "orthogonal" action, such as DB transaction I mean:

def addNewUser(name: String, surname: String, email: String): Box[User] = {
  try {
    DB.createNewTransaction()
    val user = User(name, surname, email)
    repository.persist(user)
    eventBus.publish(NewUserCreatedEvent(user.id))
 } finally {
   DB.flushTransaction()
 }
} 

^ Here, that try - finally block with all DB calls is orthogonal to business logic of this method. It shouldn't be here together with business logic code. That's a typical case when we can use Context (or CommonLoanWrapper). In Java world, it would be probably just an annotation + aspect.

*
* This is similar to [[net.liftweb.common.CommonLoanWrapper]], however, it decorates the
* function eagerly. This way, you can access current thread's state which is essential
* to set up e.g. HTTP session wrapper.

This comment has been minimized.

@farmdawgnation

farmdawgnation Oct 8, 2016

Member

This would read better as which is essential to do things like set up an HTTP session wrapper

* that current execution thread for chained method has its own request or session available if reading/writing
* some data to it as a part of chained method execution.
*/
def withCurrentSession[T](task: => T, scheduler: LAScheduler = LAScheduler): LAFuture[T] = {

This comment has been minimized.

@farmdawgnation

farmdawgnation Oct 8, 2016

Member

Maybe LAFuture.withCurrentSession should be an alias to this function?

This comment has been minimized.

@pdyraga

pdyraga Oct 10, 2016

The problem is that lift-actor does not "see" lift-webkit - dependency has an opposite direction.

This comment has been minimized.

@Shadowfiend

Shadowfiend Nov 16, 2016

Member

Is there perhaps something we can do with an implicit class here? Or perhaps we instead do something in S, like S.sessionFuture?

This comment has been minimized.

@pdyraga

pdyraga Nov 28, 2016

Good point. I added S.sessionFuture as an alias to LAFutureWithSession.withCurrentSession.

withSession(task, scheduler)
case empty: EmptyBox =>
withFailure(empty ?~! "LiftSession not available in this thread context", scheduler)

This comment has been minimized.

@farmdawgnation

farmdawgnation Oct 8, 2016

Member

Don't we normally throw an exception if something like this attempts to access the session without a proper session context? I'd prefer we keep that behavior consistent. It feels like a situation where an exception is more or less appropriate.

This comment has been minimized.

@pdyraga

pdyraga Oct 10, 2016

I'd say that sometimes we throw an exception if method needs a session but it's not available, just like S ? do, but when something like this attempts to access the session without a proper session context we usually return a Failure. This is also similar for Req.

If you take a look at S, you'll notice that S.session returns a Box. As a result, almost all functions requiring session to be available return Box as well (e.g. findOrCreateComet, getSessionAttribute, runTemplate, eval). Functions returning Boolean in S usually return false if session is not available (e.g. legacyIeCompatibilityMode). I'd rather say that S.? is an exception here.

Similar situation is for S.request. It returns Box and as a result other methods requiring request to be available return Box as well (uri, queryString, getRequestHeader, referer).

This comment has been minimized.

@farmdawgnation
val future = FutureWithSession.withCurrentSession("d")
val mapped = future
.flatMap { s => val out = s + SessionVar1.is; Future(out) }
.flatMap { s => val out = s + SessionVar2.is; Future(out) }

This comment has been minimized.

@farmdawgnation

farmdawgnation Oct 8, 2016

Member

Semicolons? In Scala? :P

This comment has been minimized.

@pdyraga

pdyraga Oct 10, 2016

I find tests with chained methods more concise when putting entire action in a single line, but if that semicolon hurts your eyes badly I can remove it 😉

This comment has been minimized.

@farmdawgnation

farmdawgnation Nov 18, 2016

Member

It's just that it's usually a code smell. 😬

Also, isn't this basically the same as a for-comp?

This comment has been minimized.

@pdyraga

pdyraga Nov 29, 2016

I forgot to add: semicolons removed :)

@pdyraga

This comment has been minimized.

pdyraga commented Oct 10, 2016

I've run all specs created in this PR dozen of times but, as usually, with tests running code concurrently, they can give different results on another machine. Good that it failed here, not later. I think I eliminated all possible issues. Some of them were related to the possible race condition that we discuss here https://groups.google.com/forum/#!topic/liftweb/V1pWy14Wl3A and some of them were my fault in implementation.

Anyway, it's ready for another chance now.

@Shadowfiend Shadowfiend referenced this pull request Nov 16, 2016

Closed

Future with session #1730

@Shadowfiend

This comment has been minimized.

Member

Shadowfiend commented Nov 16, 2016

Not as a blocker, but I wonder if we also want to make Lift's default handling of Future/LAFuture in the RHS of CSS selector transforms automatically lift the futures into a session/req context... And same for RestHelper.

pdyraga added some commits Nov 28, 2016

Drop semicolons from specs
Use standard scala formatting instead
@pdyraga

This comment has been minimized.

pdyraga commented Nov 28, 2016

Not as a blocker, but I wonder if we also want to make Lift's default handling of Future/LAFuture in the RHS of CSS selector transforms automatically lift the futures into a session/req context... And same for RestHelper.

I am not sure about this one. Although this could be handy for developers, it might also cause some bugs and confusion.

For example:

// this is fine - right side of `#>` is computed lazily so we 
// can wrap `LAFuture` with session before `SessionVar.is` gets executed
".content" #> LAFuture { SessionVar.is }
// here, `SessionVar.is` will be executed before we wrap 
// `LAFuture` in session-aware context
val future = LAFuture { SessionVar.is }
".content" #> future

The difference in code is small but serious in execution result. I'd rather prefer if users have to do

".content" #> S.sessionFuture { SessionVar.is }

or

val future = S.sessionFuture { SessionVar.is }
".content" #> future

No automatic wrapping, user has to do it explicitly, but result is predictable.

@pdyraga

This comment has been minimized.

pdyraga commented Nov 28, 2016

@farmdawgnation @Shadowfiend This is ready for another look.

@pdyraga

This comment has been minimized.

pdyraga commented Dec 1, 2016

BTW: Codacy check is not 100% accurate. I can't apply some suggested changes because the code will stop compiling. Also, I need to adjust to API that we already have, so switching the order of parameters is not an option here.

@farmdawgnation

This comment has been minimized.

Member

farmdawgnation commented Dec 6, 2016

Sorry I lost the thread on this a little bit. Hopefully I'll have time to take a look tomorrow. Leaving this in my email inbox until I do. :)

@Shadowfiend

This comment has been minimized.

Member

Shadowfiend commented Dec 6, 2016

Re: codacy check, yeah, I noticed. I ignored several of them and removed another few patterns. Mostly leaves the private[this] suggestions.

@Shadowfiend

This comment has been minimized.

Member

Shadowfiend commented Dec 6, 2016

And re: auto-wrapping vs being explicit---good point, I'm 💯 on the current solution.

@farmdawgnation

@pdyraga Sorry for the delay in getting this feedback to you. I think we're down to minor stylistic details. I think I have finally groked the code enough to understand what's going on. It only took me two months to find the time in my schedule to get Lift in my brain for a bit. 😄

(In fairness, I did get marries in that time frame so shrug haha)

toDo = Nil
onFailure = Nil
onComplete.foreach(f => LAFuture.executeWithObservers(scheduler, () => f(Full(value))))
onComplete = Nil
ret

This comment has been minimized.

@farmdawgnation
case e: EmptyBox => ret.complete(e)
Box.tryo(contextFn(v)) match {
case Full(successfullyComputedFuture) => successfullyComputedFuture.onComplete(v2 => result.complete(v2))
case e: EmptyBox => result.complete(e)

This comment has been minimized.

@farmdawgnation

farmdawgnation Dec 10, 2016

Member

Can we break these case statements up so their result is on the next line?

}
case e: EmptyBox => ret.complete(e)
case e: EmptyBox => result.complete(e)

This comment has been minimized.

@farmdawgnation

farmdawgnation Dec 10, 2016

Member

Ditto here.

* `FutureWithSession`, thus, they can be all chained together.
*
* It's important to bear in mind that each chained method requires current thread's `LiftSession` to be available.
* `FutureWithSession` does _not_ propagate initial session or request to all chained methods.

This comment has been minimized.

@farmdawgnation

farmdawgnation Dec 10, 2016

Member

What happens if that's not available?

case _ =>
new FutureWithSession(Future.failed[T](
new IllegalStateException("LiftSession not available in this thread context")
))

This comment has been minimized.

@farmdawgnation

farmdawgnation Dec 10, 2016

Member

Ah I see. Okay, that's sensible.

withSession(task, scheduler)
case empty: EmptyBox =>
withFailure(empty ?~! "LiftSession not available in this thread context", scheduler)

This comment has been minimized.

@farmdawgnation
@farmdawgnation

This comment has been minimized.

Member

farmdawgnation commented Dec 10, 2016

LGTM. @joescii @Shadowfiend Do y'all want to take a closer look at this before merge?

@Shadowfiend

This comment has been minimized.

Member

Shadowfiend commented Dec 12, 2016

Let me try and give it another pass this week, and if I don't get to it then let's just send it in.

@pdyraga

This comment has been minimized.

pdyraga commented Dec 21, 2016

Folks, I'd appreciate if you could clone this branch and run modified tests on your boxes before merging. They pass on my box, they pass on Jenkins, but since these are tests operating on multiple threads having additional checks would boost my confidence about them :)

@farmdawgnation

This comment has been minimized.

Member

farmdawgnation commented Dec 21, 2016

Seems reasonable. :)

@farmdawgnation

This comment has been minimized.

Member

farmdawgnation commented Dec 22, 2016

Ran the tests a bunch of times and didn't run into any issues. I'm merging this.

@farmdawgnation farmdawgnation merged commit 58b84b6 into lift:master Dec 22, 2016

1 of 2 checks passed

codacy/pr Not so good... This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pdyraga

This comment has been minimized.

pdyraga commented Dec 22, 2016

❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment