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

CometActor hides errors that are part of construction. #1642

Closed
gvdm opened this Issue Nov 24, 2014 · 8 comments

Comments

Projects
None yet
4 participants
@gvdm

gvdm commented Nov 24, 2014

As per the discussion at https://groups.google.com/forum/#!topic/liftweb/Ed2MfRUYlaM it has been found that if there is an exception/error as part of CometActor initialization an obscure NoSuchMethodError is instead given which hides the actual cause of the problem.

Some extra/accurate debugging information would go a long way towards reducing pain.

@Shadowfiend Shadowfiend added this to the 3.1 milestone Jan 26, 2016

@Shadowfiend

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Apr 22, 2017

Member

This could be either the same as or a subset of #1799 . Once 1799 is fixed we should revisit here and see if that is sufficient.

Durrrr… Issue says comets, I read snippets. Whoops. 1799 is a similar issue, but with respect to snippets.

Member

Shadowfiend commented Apr 22, 2017

This could be either the same as or a subset of #1799 . Once 1799 is fixed we should revisit here and see if that is sufficient.

Durrrr… Issue says comets, I read snippets. Whoops. 1799 is a similar issue, but with respect to snippets.

@Shadowfiend Shadowfiend modified the milestones: 3.1.0-M3, 3.1.0-RC1 Apr 22, 2017

@Shadowfiend

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Apr 22, 2017

Member

Want to at least see if I can figure out the cause here by M3, as it feels like it's probably an easy fix once that's done.

Member

Shadowfiend commented Apr 22, 2017

Want to at least see if I can figure out the cause here by M3, as it feels like it's probably an easy fix once that's done.

@Shadowfiend

This comment has been minimized.

Show comment
Hide comment
@Shadowfiend

Shadowfiend Apr 22, 2017

Member

Ok, so this will require a little more elbow grease than some of the other easy tasks, but it should also be fairly straightforward.

The problem here is at https://github.com/lift/framework/blob/master/web/webkit/src/main/scala/net/liftweb/http/LiftSession.scala#L2589 and https://github.com/lift/framework/blob/master/web/webkit/src/main/scala/net/liftweb/http/LiftSession.scala#L2600 . There, we invoke newInstance on a constructor we should know exists, but if it throws an exception it will be a NoSuchMethodError with an attached cause corresponding to the actual exception in question. So if an exception is thrown during comet actor construction, it will bubble up and make it appear as if a constructor of the appropriate type doesn't actually exist.

In fact, if the constructor doesn't exist I think we would get a NullPointerException, because the result of getConstructor would be null.

Things needed here:

  • Confirming that getConstructor and friends would return null in case of the relevant constructor not being present.
  • Adjusting the error handling code to:
    • Immediately fail with the “couldn't find a valid comet constructor” error if the constructor is found to be null.
    • Fail with a more descriptive error if invoking the constructor results in a down-the-line exception due to exceptions being thrown in the actual comet constructor.

We'll need some trial and error here to devise what our preferred behaviors are in various scenarios, but all the scenarios are pretty easy (constructors exist and work, constructors don't exist, constructors exist and throw exceptions) to go through.

Member

Shadowfiend commented Apr 22, 2017

Ok, so this will require a little more elbow grease than some of the other easy tasks, but it should also be fairly straightforward.

The problem here is at https://github.com/lift/framework/blob/master/web/webkit/src/main/scala/net/liftweb/http/LiftSession.scala#L2589 and https://github.com/lift/framework/blob/master/web/webkit/src/main/scala/net/liftweb/http/LiftSession.scala#L2600 . There, we invoke newInstance on a constructor we should know exists, but if it throws an exception it will be a NoSuchMethodError with an attached cause corresponding to the actual exception in question. So if an exception is thrown during comet actor construction, it will bubble up and make it appear as if a constructor of the appropriate type doesn't actually exist.

In fact, if the constructor doesn't exist I think we would get a NullPointerException, because the result of getConstructor would be null.

Things needed here:

  • Confirming that getConstructor and friends would return null in case of the relevant constructor not being present.
  • Adjusting the error handling code to:
    • Immediately fail with the “couldn't find a valid comet constructor” error if the constructor is found to be null.
    • Fail with a more descriptive error if invoking the constructor results in a down-the-line exception due to exceptions being thrown in the actual comet constructor.

We'll need some trial and error here to devise what our preferred behaviors are in various scenarios, but all the scenarios are pretty easy (constructors exist and work, constructors don't exist, constructors exist and throw exceptions) to go through.

@Shadowfiend Shadowfiend modified the milestones: 3.1.0-M3, 3.1.0-RC1 May 20, 2017

@Shadowfiend Shadowfiend modified the milestones: 3.1.0-RC1, 3.2.0-M1 Jun 15, 2017

@farmdawgnation farmdawgnation modified the milestones: 3.2.0-M1, 3.2.0-M2 Jul 22, 2017

@farmdawgnation

This comment has been minimized.

Show comment
Hide comment
@farmdawgnation

farmdawgnation Sep 9, 2017

Member

Well I just got bit by this error pretty hard while working on the Judicial Manager code.

scala> res0.getConstructor().newInstance()
java.lang.NullPointerException
  at net.liftweb.sitemap.Loc.siteMap(Loc.scala:157)
  at net.liftweb.sitemap.Loc.siteMap$(Loc.scala:157)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.siteMap(Menu.scala:177)
  at net.liftweb.sitemap.Loc.allParams(Loc.scala:133)
  at net.liftweb.sitemap.Loc.allParams$(Loc.scala:130)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.allParams(Menu.scala:177)
  at net.liftweb.sitemap.Loc.net$liftweb$sitemap$Loc$$staticValue(Loc.scala:88)
  at net.liftweb.sitemap.Loc.net$liftweb$sitemap$Loc$$staticValue$(Loc.scala:87)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.net$liftweb$sitemap$Loc$$staticValue$lzycompute(Menu.scala:177)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.net$liftweb$sitemap$Loc$$staticValue(Menu.scala:177)
  at net.liftweb.sitemap.Loc.$anonfun$paramValue$2(Loc.scala:85)
  at net.liftweb.common.EmptyBox.or(Box.scala:871)
  at net.liftweb.sitemap.Loc.paramValue(Loc.scala:85)
  at net.liftweb.sitemap.Loc.paramValue$(Loc.scala:85)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.paramValue(Menu.scala:177)
  at net.liftweb.sitemap.Loc.$anonfun$currentValue$3(Loc.scala:124)
  at net.liftweb.common.EmptyBox.or(Box.scala:871)
  at net.liftweb.sitemap.Loc.currentValue(Loc.scala:124)
  at net.liftweb.sitemap.Loc.currentValue$(Loc.scala:124)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.currentValue(Menu.scala:177)
  at frmr.scyig.webapp.comet.ScheduleEditor.<init>(ScheduleEditor.scala:21)

I had an NPE in my comet constructor (werps) and it gave me a constructor error. Did some testing in the REPL for fun... it looks like the reflection code does throw a proper NoSuchMethodException if the constructor doesn't exist:

scala> class TheBacon(string: String)
defined class TheBacon

scala> classOf[TheBacon]
res3: Class[TheBacon] = class TheBacon

scala> res3.getConstructor().newInstance()
java.lang.NoSuchMethodException: TheBacon.<init>()
  at java.lang.Class.getConstructor0(Class.java:3082)
  at java.lang.Class.getConstructor(Class.java:1825)
  ... 39 elided

There's no wrapping of underlying exceptions that occur from what I can tell.

I think this is as simple as changing this line to only evaluate buildWithCreateInfoConstructor if the exception from the first tryo was a NoSuchMethodException.

Member

farmdawgnation commented Sep 9, 2017

Well I just got bit by this error pretty hard while working on the Judicial Manager code.

scala> res0.getConstructor().newInstance()
java.lang.NullPointerException
  at net.liftweb.sitemap.Loc.siteMap(Loc.scala:157)
  at net.liftweb.sitemap.Loc.siteMap$(Loc.scala:157)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.siteMap(Menu.scala:177)
  at net.liftweb.sitemap.Loc.allParams(Loc.scala:133)
  at net.liftweb.sitemap.Loc.allParams$(Loc.scala:130)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.allParams(Menu.scala:177)
  at net.liftweb.sitemap.Loc.net$liftweb$sitemap$Loc$$staticValue(Loc.scala:88)
  at net.liftweb.sitemap.Loc.net$liftweb$sitemap$Loc$$staticValue$(Loc.scala:87)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.net$liftweb$sitemap$Loc$$staticValue$lzycompute(Menu.scala:177)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.net$liftweb$sitemap$Loc$$staticValue(Menu.scala:177)
  at net.liftweb.sitemap.Loc.$anonfun$paramValue$2(Loc.scala:85)
  at net.liftweb.common.EmptyBox.or(Box.scala:871)
  at net.liftweb.sitemap.Loc.paramValue(Loc.scala:85)
  at net.liftweb.sitemap.Loc.paramValue$(Loc.scala:85)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.paramValue(Menu.scala:177)
  at net.liftweb.sitemap.Loc.$anonfun$currentValue$3(Loc.scala:124)
  at net.liftweb.common.EmptyBox.or(Box.scala:871)
  at net.liftweb.sitemap.Loc.currentValue(Loc.scala:124)
  at net.liftweb.sitemap.Loc.currentValue$(Loc.scala:124)
  at net.liftweb.sitemap.Menu$ParamMenuable$$anon$9.currentValue(Menu.scala:177)
  at frmr.scyig.webapp.comet.ScheduleEditor.<init>(ScheduleEditor.scala:21)

I had an NPE in my comet constructor (werps) and it gave me a constructor error. Did some testing in the REPL for fun... it looks like the reflection code does throw a proper NoSuchMethodException if the constructor doesn't exist:

scala> class TheBacon(string: String)
defined class TheBacon

scala> classOf[TheBacon]
res3: Class[TheBacon] = class TheBacon

scala> res3.getConstructor().newInstance()
java.lang.NoSuchMethodException: TheBacon.<init>()
  at java.lang.Class.getConstructor0(Class.java:3082)
  at java.lang.Class.getConstructor(Class.java:1825)
  ... 39 elided

There's no wrapping of underlying exceptions that occur from what I can tell.

I think this is as simple as changing this line to only evaluate buildWithCreateInfoConstructor if the exception from the first tryo was a NoSuchMethodException.

@farmdawgnation

This comment has been minimized.

Show comment
Hide comment
@farmdawgnation

farmdawgnation Sep 9, 2017

Member

FWIW, it also looks like I got bit by an NPE somewhere in the sitemap code, but this should have still bubbled correctly. :)

Member

farmdawgnation commented Sep 9, 2017

FWIW, it also looks like I got bit by an NPE somewhere in the sitemap code, but this should have still bubbled correctly. :)

@andreak

This comment has been minimized.

Show comment
Hide comment
@andreak

andreak Sep 15, 2017

Member

Yea. Loc:157

  def siteMap: SiteMap = _menu.siteMap

and Loc:404

  def breadCrumbs: List[Loc[_]] = _menu.breadCrumbs ::: List(this)

Are the places where I occationally run into these NPEs, as mentioned:
https://groups.google.com/forum/#!searchin/liftweb/Frequent$20NullPointerException$20in$20Loc.breadCrumbs%7Csort:relevance/liftweb/7KpP-NDcTd8/hHwQOpgoAgAJ

But, only in dev-mode. Have you gotten these in prod-mode?

Member

andreak commented Sep 15, 2017

Yea. Loc:157

  def siteMap: SiteMap = _menu.siteMap

and Loc:404

  def breadCrumbs: List[Loc[_]] = _menu.breadCrumbs ::: List(this)

Are the places where I occationally run into these NPEs, as mentioned:
https://groups.google.com/forum/#!searchin/liftweb/Frequent$20NullPointerException$20in$20Loc.breadCrumbs%7Csort:relevance/liftweb/7KpP-NDcTd8/hHwQOpgoAgAJ

But, only in dev-mode. Have you gotten these in prod-mode?

@farmdawgnation

This comment has been minimized.

Show comment
Hide comment
@farmdawgnation

farmdawgnation Sep 15, 2017

Member

I didn't try flipping it to prod mode, because that doesn't strike me as something that should change behavior? But maybe it is? O_o

Member

farmdawgnation commented Sep 15, 2017

I didn't try flipping it to prod mode, because that doesn't strike me as something that should change behavior? But maybe it is? O_o

@andreak

This comment has been minimized.

Show comment
Hide comment
@andreak

andreak Sep 15, 2017

Member

That's the strange part. It's excactly that which makes the difference; I only experience this in dev-mode and it probably has something to do with a race-condition building the menu each time, which only happens in dev-mode.

Member

andreak commented Sep 15, 2017

That's the strange part. It's excactly that which makes the difference; I only experience this in dev-mode and it probably has something to do with a race-condition building the menu each time, which only happens in dev-mode.

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