Skip to content

Add LiftSession.onFunctionOwnersRemoved.#1709

Merged
Shadowfiend merged 2 commits into
masterfrom
scope-callback
Jul 9, 2015
Merged

Add LiftSession.onFunctionOwnersRemoved.#1709
Shadowfiend merged 2 commits into
masterfrom
scope-callback

Conversation

@Shadowfiend
Copy link
Copy Markdown
Member

These functions are invoked with the set of function owners that had
all of their remaining associated functions evicted from a session in a
given run. A function owner is typically a page’s RenderVersion, though
there are cases (e.g., comets) where this is not necessarily the case.

@andreak have a first look and see what you think!

These functions are invoked with the set of function owners that had
all of their remaining associated functions evicted from a session in a
given run. A function owner is typically a page’s RenderVersion, though
there are cases (e.g., comets) where this is not necessarily the case.
@andreak
Copy link
Copy Markdown
Member

andreak commented Jul 3, 2015

Cool!
I'll have a look at it tomorrow, it's getting late here...

0.13.5 seems to be having alllll sorts of issues.
@andreak
Copy link
Copy Markdown
Member

andreak commented Jul 4, 2015

My thoughts was that one could, in a Snippet or comet, register a function for the given S.renderVersion, kind of like this:

S.registerFunctionOwnersRemoved((renderVerson: String) => {
    println(s"Removed $renderVerson and all references in database")
})

and that S.registerFunctionOwnersRemoved used S.renderVersion behind the scenes and sent that as the argument in to the registered function.

When the function is executed it must also be removed from the list of registered functions, or else we accumulate lots of these and leak memory.

I see that onFunctionOwnersRemoved has the following signature:
var onFunctionOwnersRemoved: List[Set[String] => Unit] = Nil

Can you elaborate on how you think this should be used?

Thanks!

@Shadowfiend
Copy link
Copy Markdown
Member Author

This is basically a lower-level version of that, and leaves management of which render versions to watch to the implementing function.

Basic use:

// Boot.scala
LiftSession.onFunctionOwnersRemoved ::= { removedOwners =>
  println(removedOwners) // prints all owners who were removed for any session, when it happens
}

The key here is that not all owners are render versions, and some owners have session lifetime. For example, inside a comet, function ownership is a little stranger, from my brief investigation while I did this work. Functions bound in comets may have session lifetime unless you explicitly instruct otherwise, if I'm not mistaken.

The most straightforward way of achieving the thing you want to with the above API is:

object pageIdWatchers extends java.util.concurrent.ConcurrentHashMap[String,()=>Unit]

// Boot.scala
LiftSession.onFunctionOwnersRemoved ::= { removedOwners =>
  for {
    owner <- removedOwners
    watcher <- Option(pageIdWatchers.get(owner))
  } yield {
    watcher()
  }
}

// ...
// snippet
def cleanupPage = {
  cachedThingies.foreach(_.remove)
}
pageIdWatchers.put(S.renderVersion, cleanupPage)

@andreak
Copy link
Copy Markdown
Member

andreak commented Jul 4, 2015

Cool, I'll try it out later.

@andreak
Copy link
Copy Markdown
Member

andreak commented Jul 5, 2015

This is what I ended up with:

object pageIdWatchers extends java.util.concurrent.ConcurrentHashMap[String,List[()=>Unit]] {
    def addCleanupFunc(func: () => Unit): Unit = {
        put(S.renderVersion, List(func) ::: pageIdWatchers.getOrDefault(S.renderVersion, Nil))
    }
}

In Boot:

LiftSession.onFunctionOwnersRemoved ::= { removedOwners =>
    for {
        owner <- removedOwners
        watcher <- Option(pageIdWatchers.get(owner))} {
        watcher.foreach(_())
        pageIdWatchers.remove(owner)
    }
}

In my snippet:

def cleanupPage(): Unit = {
    println(s"Running cleanupPage for ${S.renderVersion}.")
}
println(s"Adding cleanupFunc for ${S.renderVersion}")
pageIdWatchers.addCleanupFunc(cleanupPage)

AFAICS this fully solves my use-case, thanks!

@Shadowfiend
Copy link
Copy Markdown
Member Author

A piece of advice to that: I would make addCleanupFunc wrap the passed function in a function runs it then removes it from the map, that way the caller doesn't need to worry about it.

That said, rock on! Another thing worth noting: I'm not sure what S.renderVersion does in weird cases (e.g., no S context, other contexts, etc).

We'll give this a couple of days for feedback and then merge it into master, I think!

@andreak
Copy link
Copy Markdown
Member

andreak commented Jul 6, 2015

As a developer might call pageIdWatchers.addCleanupFunc multiple times in a snippet there might be a list of functions registered as callbacks for the given ownerID, hence the signature ConcurrentHashMap[String,List[()=>Unit]]. Removing a function from this list for the ownerID-key when the function is finished and testing if there are any functions left for then removing the key introduces unnecessary complexity in addCleanupFunc I think. The implementation of onFunctionOwnersRemoved in Boot removes the ownerID-key, removing all the functions after finished executing. I feel that the burden of removing the funcs is sufficiently removed from the user this way.

In Comet S.renderVersion returns nextFuncName which is useless in this context. What is the removedOwners List[String] for functions registered in Comet-context, and how to I obtain them in addCleanupFunc?

@andreak
Copy link
Copy Markdown
Member

andreak commented Jul 6, 2015

After testing more from CometActors, which doesn't bind AJAX-funcs using S.renderVersion, I found out that the AJAX-funcs' owner is the CometActor.uniqueId, so this is a better suited implementation for addCleanupFunc:

object pageIdWatchers extends java.util.concurrent.ConcurrentHashMap[String,List[()=>Unit]] {
    def addCleanupFunc(func: () => Unit): Unit = {
        S.currentCometActor.map(_.uniqueId).orElse(S.request.map(_ => S.renderVersion)).foreach(functionOwner =>
            put(functionOwner, List(func) ::: pageIdWatchers.getOrDefault(functionOwner, Nil))
        )
    }
}

@Shadowfiend
Copy link
Copy Markdown
Member Author

Sweet, sounds good. I'm going to put a note on the relevant ML thread about all of this, it'd be cool if you mentioned your implementation of pageIdWatchers and use thereof on that thread as well so there's a reference for a possible use.

@andreak
Copy link
Copy Markdown
Member

andreak commented Jul 6, 2015

Thanks, I will.

@andreak
Copy link
Copy Markdown
Member

andreak commented Jul 9, 2015

Merge-time?

@Shadowfiend
Copy link
Copy Markdown
Member Author

Yep yep, thanks for the reminder :)

Shadowfiend added a commit that referenced this pull request Jul 9, 2015
Add LiftSession.onFunctionOwnersRemoved.

These functions are invoked with the set of function owners that had
all of their remaining associated functions evicted from a session in a
given run. A function owner is typically a page’s RenderVersion, though
there are cases (e.g., comets) where this is not necessarily the case.

As a sample usage:

object pageIdWatchers extends java.util.concurrent.ConcurrentHashMap[String,List[()=>Unit]] {
    def addCleanupFunc(func: () => Unit): Unit = {
        put(S.renderVersion, List(func) ::: pageIdWatchers.getOrDefault(S.renderVersion, Nil))
    }
}

In Boot:

LiftSession.onFunctionOwnersRemoved ::= { removedOwners =>
    for {
        owner <- removedOwners
        watcher <- Option(pageIdWatchers.get(owner))} {
        watcher.foreach(_())
        pageIdWatchers.remove(owner)
    }
}

In a snippet:

def cleanupPage(): Unit = {
    println(s"Running cleanupPage for ${S.renderVersion}.")
}
println(s"Adding cleanupFunc for ${S.renderVersion}")
pageIdWatchers.addCleanupFunc(cleanupPage)
@Shadowfiend Shadowfiend merged commit 11c5825 into master Jul 9, 2015
@Shadowfiend Shadowfiend deleted the scope-callback branch July 9, 2015 14:27
@fmpwizard fmpwizard added this to the 3.0-M6 milestone Jul 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants