Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

A couple of LiftScreen fixes #1415

Closed
wants to merge 3 commits into from

4 participants

pbrant Diego Medina Antonio Salazar Cardozo nafg
pbrant
Collaborator

Fixes for a couple of fairly esoteric (CssBound)LiftScreen bug fixes. Heavily tested and in production.

pbrant added some commits
pbrant pbrant Use mutable Map for storing local actions
If multiple Ajax requests are submitted at once, they'll get independent
copies of the LocalActions map.  This may result in subsequent errors if
local actions created by e.g. the first request are used later in
combination with a snapshot created by a later request.
b1fb18d
pbrant pbrant LocalActionRef should be a RequestVar, not ScreenVar
The lifetime of a ScreenVar may extend across multiple pages, but the
contents of this particular ScreenVar is inherently request scoped.
Local actions may cease working if the screen is restored from an old
snapshot.
c9ec788
.../src/main/scala/net/liftweb/http/CssBoundScreen.scala
@@ -97,7 +99,7 @@ trait CssBoundScreen extends ScreenWizardRendered with Loggable {
protected def mapLocalAction[T](func: () => JsCmd)(f: String => T): T = {
val name = randomString(20)
- LocalActions.set(LocalActions.is + (name -> func))
+ LocalActions.get += (name -> func)
Diego Medina Owner

Is there any risk of race conditions changing the Map for a mutable Map ?

pbrant Collaborator
pbrant added a note
Antonio Salazar Cardozo Owner

I will let you judge if this affects that, but keep in mind that a slow Ajax request will time out on the client, at which point the client is free to fire off another request. Exponential backoff means that request may not be a retry (a retry won't rerun the code anymore), so you can end up with two or more requests running from a given session at a given moment.

nafg
nafg added a note
pbrant Collaborator
pbrant added a note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Diego Medina
Owner

Tonight I'll be spinning RC2 with bcrypt added to mapper, if you guys think this one is good to go, I could include it too

pbrant
Collaborator

Commit above fixes the race condition (which apparently can happen, although the chances should be very small). Should be good to go from my perspective.

Diego Medina
Owner

rebased to master

Diego Medina fmpwizard closed this
pbrant pbrant deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 26, 2013
  1. pbrant

    Use mutable Map for storing local actions

    pbrant authored
    If multiple Ajax requests are submitted at once, they'll get independent
    copies of the LocalActions map.  This may result in subsequent errors if
    local actions created by e.g. the first request are used later in
    combination with a snapshot created by a later request.
  2. pbrant

    LocalActionRef should be a RequestVar, not ScreenVar

    pbrant authored
    The lifetime of a ScreenVar may extend across multiple pages, but the
    contents of this particular ScreenVar is inherently request scoped.
    Local actions may cease working if the screen is restored from an old
    snapshot.
Commits on Mar 5, 2013
  1. pbrant
This page is out of date. Refresh to see the latest.
8 web/webkit/src/main/scala/net/liftweb/http/CssBoundLiftScreen.scala
View
@@ -22,6 +22,7 @@ import net.liftweb.common._
import util._
import xml._
+import java.util.concurrent.atomic.AtomicReference
trait CssBoundLiftScreen extends LiftScreen with CssBoundScreen {
protected object SavedDefaultXml extends ScreenVar[NodeSeq](defaultXml) {
@@ -32,7 +33,7 @@ trait CssBoundLiftScreen extends LiftScreen with CssBoundScreen {
override lazy val __nameSalt = Helpers.nextFuncName
}
- protected object LocalActionRef extends ScreenVar[String](S.fmapFunc(setLocalAction _)(s => s)) {
+ protected object LocalActionRef extends RequestVar[String](S.fmapFunc(setLocalAction _)(s => s)) {
override lazy val __nameSalt = Helpers.nextFuncName
}
@@ -44,7 +45,8 @@ trait CssBoundLiftScreen extends LiftScreen with CssBoundScreen {
override lazy val __nameSalt = Helpers.nextFuncName
}
- protected object LocalActions extends ScreenVar[Map[String, () => JsCmd]](Map[String, () => JsCmd]()) {
+ protected object LocalActions extends ScreenVar[AtomicReference[Map[String, () => JsCmd]]](
+ new AtomicReference[Map[String, () => JsCmd]](Map.empty)) {
override lazy val __nameSalt = Helpers.nextFuncName
}
@@ -56,7 +58,7 @@ trait CssBoundLiftScreen extends LiftScreen with CssBoundScreen {
protected def savedDefaultXml = SavedDefaultXml.get
override protected def doFinish(): JsCmd= {
- val fMap: Map[String, () => JsCmd] = LocalActions.get
+ val fMap: Map[String, () => JsCmd] = LocalActions.get.get
if (! LocalAction.get.isEmpty)
fMap.get(LocalAction.get) map (_()) getOrElse (
throw new IllegalArgumentException("No local action available with that binding"))
9 web/webkit/src/main/scala/net/liftweb/http/CssBoundScreen.scala
View
@@ -28,6 +28,8 @@ import xml._
import FieldBinding._
+import java.util.concurrent.atomic.AtomicReference
+
trait CssBoundScreen extends ScreenWizardRendered with Loggable {
self: AbstractScreen =>
@@ -39,7 +41,7 @@ trait CssBoundScreen extends ScreenWizardRendered with Loggable {
protected val LocalActionRef: AnyVar[String, _]
protected val LocalAction: AnyVar[String, _]
- protected val LocalActions: AnyVar[Map[String, () => JsCmd], _]
+ protected val LocalActions: AnyVar[AtomicReference[Map[String, () => JsCmd]], _]
val NextId: AnyVar[String, _]
@@ -97,7 +99,10 @@ trait CssBoundScreen extends ScreenWizardRendered with Loggable {
protected def mapLocalAction[T](func: () => JsCmd)(f: String => T): T = {
val name = randomString(20)
- LocalActions.set(LocalActions.is + (name -> func))
+ val ref = LocalActions.get
+ ref.synchronized {
+ ref.set(ref.get + (name -> func))
+ }
f(name)
}
Something went wrong with that request. Please try again.