NPE in S.?! if used outside a request

[Ross Mellgren]

I'm poking around the LiftTicket codebase, and currently it suffers from an NPE on Boot:

ERROR - Failed to Boot
java.lang.ExceptionInInitializerError
at org.liftticket.liftticket.snippet.UserAdmin$.menus(UserAdmin.scala:41)
at bootstrap.liftweb.Boot.buildMenus(Boot.scala:74)
at bootstrap.liftweb.Boot.boot(Boot.scala:67)
...
Caused by: java.lang.NullPointerException
at net.liftweb.http.S$.$qmark$bang(S.scala:675)
at net.liftweb.http.S$.$qmark(S.scala:636)
at org.liftticket.liftticket.snippet.RoleAdmin$.<init>(RoleAdmin.scala:94)
at org.liftticket.liftticket.snippet.RoleAdmin$.<clinit>(RoleAdmin.scala)
... 57 more

This is apparently due to S._resBundle being null because S.init has not yet been entered, so S._resBundle.doWith(Nil) is not in the enclosing scope, and so:

private def ?!(str: String, resBundle: List[ResourceBundle]): String = resBundle.flatMap(r => tryo(r.getObject(str) match {

Blows up with NPE. resBundle is passed in from S.? by calling S.resourceBundles, which is defined thus:

def resourceBundles: List[ResourceBundle] = {
_resBundle.value match {
case Nil => {
// loads the resource bundles here
}
case bundles => bundles
}
}

I don't think the resource bundles can be reasonably loaded outside a request since no locale can be computed -- the call site should lazily compute the menu text at request time using LinkText rather than eagerly using S.?, but the error case is not helpful. What I propose is changing S.resourceBundles like this:

def resourceBundles: List[ResourceBundle] = {
_resBundle.box match {
case Full(Nil) => {
// same code
}
case Full(bundles) => bundles
case _ => {
Log.error("Resource bundles cannot be loaded outside of a request scope since the current locale cannot be computed. Did you use S.? during Boot time?")
Nil
}
}
}

-Ross

[Timothy Perrett]

Ross,

Where in your code are you calling this method? What I'd the context
that puts it outside of a request?

Cheers, Tim

Sent from my iPhone

> --
>
> You received this message because you are subscribed to the Google
> Groups "Lift" group.
> To post to this group, send email to lif...@googlegroups.com.
> To unsubscribe from this group, send email to liftweb+u...@googlegroups.com
> .
> For more options, visit this group at http://groups.google.com/group/liftweb?hl=
> .
>
>
>

[Ross Mellgren]

It's being called to provide the link text for menus during sitemap creation at boot time, as well as during class initialization time to make some vals that contain error text. I believe initializing these at Boot time to be incorrect since the correct locale cannot be known until a request is made (unless you force one locale for the entire site, I guess), but the NPE seems uglier than providing an error log explaining the problem.

-Ross

[Ross Mellgren]

Oops I misspoke, the strToLinkText conversion is already lazy, so it's not the menu text, just the message. Here is the diff:

@@ -91,7 +91,7 @@ object RoleAdmin {

case _ => Text(S.?("No instance to delete"))

}

- val missingTemplate = Text(S.?("Missing template"))

+ def missingTemplate = Text(S.?("Missing template"))

def menus = {

import Loc._

-Ross

[Timothy Perrett]

I agree that getting an NPE is less than desirable!

I'm not sure things are wrong per-say; the defualt locale calculator
has an app wide setting and there is nothing to say that that a user
must implement their locale calculator in a particular way... This is
one of the great things about lift ;-)

So the issue here seems to be the lazy / eager evaluation of bald
rather than localization out of request context? I'm all for getting
rid of these NPE scenarios though :-)

Cheers, Tim

Sent from my iPhone

[Ross Mellgren]

Hmm, you raise an interesting point. So I guess the situation is more variegated:

1) If you have a locale calculator that works per-request and does not have an app default, then it is definitely incorrect to call S.? out of a request context,
2) but if you have one that works app-wide then it should work -- but doesn't, unless you call S.init or one of those by yourself,
3) so my proposed error is not good.

It would be nice to catch uses of S.? outside of a request context when your intended behavior is that you only have per-request locales, so I guess if that is the desire then one should make their locale calculator log the error text, and the change to lift should be something more like:

def resourceBundles: List[ResourceBundle] = {

- _resBundle.value match {
- case Nil => {
+ _resBundle.box match {
+ case Full(bundles) if !bundles.isEmpty => bundles
+ case _ => {
_resBundle.set(LiftRules.resourceNames.flatMap(name => tryo(
List(ResourceBundle.getBundle(name, locale))
).openOr(
NamedPF.applyBox((name, locale), LiftRules.resourceBundleFactories.toList).map(List(_)) openOr Nil
)))
_resBundle.value
}
- case bundles => bundles
}
}

That is, if the _resBundle ThreadGlobal has not been initialized yet, or it's been initialized with the empty list, then do resource bundle loading. If it has been initialized with a non-empty list then use that.

I don't think this should interact badly with S.init, since S.init uses _resBundle.doWith(Nil) ... which will both overwrite any loaded "app-level" bundles with the empty list, but also restore them upon completion of the S scope.

-Ross

[David Pollak]

I am okay with an NPE thrown from using S outside a request.  Yeah, it'd be nicer to have a more polite error message, but there should be no creep towards being able to call S outside a request/session scope.

Also, except for tests or actors, one should never think about initializing S.

--
Lift, the simply functional web framework http://liftweb.net
Beginning Scala http://www.apress.com/book/view/1430219890
Follow me: http://twitter.com/dpp
Surf the harmonics

[Ross Mellgren]

So would you prefer no change or change to have better error message?

-Ross

[David Pollak]

On Mon, Nov 23, 2009 at 11:01 AM, Ross Mellgren <dri...@gmail.com> wrote:

So would you prefer no change or change to have better error message?

Change to have a better error message
 

[Timothy Perrett]

Ross, what did you have in mind for presenting a better error message?

Cheers, Tim

[Ross Mellgren]

Unless there's a problem with it, my original idea was:

def resourceBundles: List[ResourceBundle] = {
_resBundle.box match {
case Full(Nil) => {

// loads the resource bundles here
}

case Full(bundles) => bundles
case _ => {
Log.error("Resource bundles cannot be loaded outside of a
request scope since the current locale cannot
be computed. Did you
use S.? during Boot time?")
Nil
}
}
}

For reference, here's the original pseudo-code (pseudo because I omit
the Nil branch, as I did above:

def resourceBundles: List[ResourceBundle] = {
_resBundle.value match {
case Nil => {
// loads the resource bundles here
}
case bundles => bundles
}
}

Though perhaps the error message could be improved to reflect the
larger nature of the issue, for example:

S is not properly initialized and cannot be used in this scope. It
should only be used in a properly initialized scope, such as during
request processing.

-Ross

>>> Lift, the simply functional web framework http://liftweb.net
>>> Beginning Scala http://www.apress.com/book/view/1430219890
>>> Follow me: http://twitter.com/dpp
>>> Surf the harmonics
>>>
>>> --
>>>
>>> You received this message because you are subscribed to the Google
>>> Groups "Lift" group.
>>> To post to this group, send email to lif...@googlegroups.com.