Fix #3921: Work around JDK-6265952: MethodAccessorImpl NoClassDefError #3928

xerial · 2020-01-16T23:02:58Z

No description provided.

gzm0 · 2020-01-17T08:44:47Z

Higher level (details): I don't think we should release 1.0.0 (or 1.0.0-RC3) with this. So a workaround is not needed as this repo does not suffer from the issue.

Lower level: The reported stack-trace in #3921 suggests that we'll also need to add "jdk." for some jdks.

gzm0 · 2020-01-17T13:34:47Z

Please see #3921 (comment). tl;dr: I think this workaround is fine.

gzm0

The commit message should read something like:

"Fix #3921: Work around JDK-6265952: MethodAccessorImpl NoClassDefError"

gzm0 · 2020-01-17T13:38:00Z

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/LinkerImpl.scala

+       * https://www.ibm.com/developerworks/community/forums/html/threadTopic?id=77777777-0000-0000-0000-000013894489&ps=25
+       *
+       * So we will delegate the access to sun.* packages to the parent class loader instead of using reflection.
+       */


Please:

Put the workaround at the end of the list.

Adjust the comment as pointed out in Running multiple fastOptJS causes java.lang.NoClassDefFoundError: jdk/internal/reflect/MethodAccessorImpl #3921 (comment)

Consider only adding the prefix "sun.reflect."

Also add "jdk.internal.reflect." (judging from the stack trace in Running multiple fastOptJS causes java.lang.NoClassDefFoundError: jdk/internal/reflect/MethodAccessorImpl #3921) or verify this workaround works for the airframe CI.

So on second reading, I think this comment still needs improvement. Notably, the second section doesn't "connect up" to the first one.

Things that at the moment are unclear from the comment IMHO:

Why is "sun.reflect." there as well?

If java.lang.reflect.Method becomes invalid, why does this fix it? Why do we not need to recreate a new jlr.Method?

Its non obvious that the prefixes are implementation details of the relevant JDKs (this is related to the first point). So if somebody sees a NoClassDefFoundError: moon.reflect.MethodAccessorImpls missing, this is probably the same issue.

I think point 1 and 3 can be addressed by making it clear that this is an implementation detail and jdk.internal.reflect.MethodAccessorImpl and sun.reflect.MethodAccessorImpl are the currently known affected classes.

Point 2 is a bit more tricky: I think we need to either explain the thing in full, or omit explanation of why this happens and simply describe

trigger (call invoke sun.reflect.inflationThreshold times on same Method instance)

symptom (NoClassDefFoundError)

mitigation (add prefix here)

and refer to the bug, letting the reader reconstruct the details by themselves.

sjrd · 2020-01-17T14:02:52Z

Marking "DO NOT MERGE" until #3931 is merged.

xerial · 2020-01-17T18:38:46Z

@gzm0 Fixed the PR as suggested. And confirmed this version worked well for building Airframe's js projects all at once!

xerial · 2020-01-17T19:59:25Z

oops. Forgot to add a trailing dot to "sun.reflect". Fixed.

xerial · 2020-01-17T22:34:14Z

Fixed the formatting issue too (120 column limit)

gzm0 · 2020-01-19T07:04:44Z

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/LinkerImpl.scala

+       * https://www.ibm.com/developerworks/community/forums/html/threadTopic?id=77777777-0000-0000-0000-000013894489&ps=25
+       *
+       * So we will delegate the access to sun.* packages to the parent class loader instead of using reflection.
+       */


So on second reading, I think this comment still needs improvement. Notably, the second section doesn't "connect up" to the first one.

Things that at the moment are unclear from the comment IMHO:

Why is "sun.reflect." there as well?

If java.lang.reflect.Method becomes invalid, why does this fix it? Why do we not need to recreate a new jlr.Method?

Its non obvious that the prefixes are implementation details of the relevant JDKs (this is related to the first point). So if somebody sees a NoClassDefFoundError: moon.reflect.MethodAccessorImpls missing, this is probably the same issue.

I think point 1 and 3 can be addressed by making it clear that this is an implementation detail and jdk.internal.reflect.MethodAccessorImpl and sun.reflect.MethodAccessorImpl are the currently known affected classes.

Point 2 is a bit more tricky: I think we need to either explain the thing in full, or omit explanation of why this happens and simply describe

trigger (call invoke sun.reflect.inflationThreshold times on same Method instance)

symptom (NoClassDefFoundError)

mitigation (add prefix here)

and refer to the bug, letting the reader reconstruct the details by themselves.

xerial · 2020-01-19T07:18:37Z

@gzm0 Thanks for the comment. I still don't have the full context, including why this workaround can fix the issue, and how the current LinkerImpl works, so I think you are the best person for fixing the comment to address these new questions.

Due to the holiday season of US, I'll not be available for 2 or 3 days, so if you are available please fix the comment so that it can be helpful for future readers.

I can take a look later, but please understand my knowledge around Scala.js compiler is quite limited at this moment.

gzm0 · 2020-01-19T20:01:23Z

if you are available please fix the comment so that it can be helpful for future readers.

I have done so. Thanks for your contribution!

gzm0 · 2020-01-19T20:02:09Z

@sjrd PTAL a the comment I have edited.

sjrd · 2020-01-19T20:30:24Z

sbt-plugin/src/main/scala/org/scalajs/sbtplugin/LinkerImpl.scala

+       * The bug is triggered when calling `java.lang.Method#invoke` if both of
+       * the following conditions are met:
+       *
+       * - this call 15th (or later) time `invoke` is called on this instance,


This sentence looks broken.

Otherwise LGTM.

:-/ yes. fixed

…sDefError

xerial · 2020-01-21T18:57:18Z

@gzm0 Thanks for the fix!

This was referenced Jan 16, 2020

#3921: Workaround for MethodAccessorImpl NoClassDefError #3927

Closed

Running multiple fastOptJS causes java.lang.NoClassDefFoundError: jdk/internal/reflect/MethodAccessorImpl #3921

Closed

xerial requested a review from gzm0 January 17, 2020 00:48

gzm0 requested changes Jan 17, 2020

View reviewed changes

sjrd changed the title ~~Workaround for MethodAccessorImpl NoClassDefError (#3921)~~ DO NOT MERGE Workaround for MethodAccessorImpl NoClassDefError (#3921) Jan 17, 2020

xerial force-pushed the fix-issue-3921 branch from 25910a1 to bad4bf3 Compare January 17, 2020 18:36

sjrd changed the title ~~DO NOT MERGE Workaround for MethodAccessorImpl NoClassDefError (#3921)~~ Workaround for MethodAccessorImpl NoClassDefError (#3921) Jan 17, 2020

xerial force-pushed the fix-issue-3921 branch from bad4bf3 to 707ee85 Compare January 17, 2020 19:58

xerial force-pushed the fix-issue-3921 branch from 707ee85 to 77e418c Compare January 17, 2020 22:33

xerial requested a review from gzm0 January 18, 2020 06:49

gzm0 requested changes Jan 19, 2020

View reviewed changes

gzm0 force-pushed the fix-issue-3921 branch from 77e418c to 854ad4f Compare January 19, 2020 19:59

gzm0 changed the title ~~Workaround for MethodAccessorImpl NoClassDefError (#3921)~~ Fix #3921: Work around JDK-6265952: MethodAccessorImpl NoClassDefError Jan 19, 2020

gzm0 force-pushed the fix-issue-3921 branch from 854ad4f to a414ec9 Compare January 19, 2020 20:00

gzm0 requested a review from sjrd January 19, 2020 20:01

gzm0 approved these changes Jan 19, 2020

View reviewed changes

sjrd reviewed Jan 19, 2020

View reviewed changes

Fix scala-js#3921: Work around JDK-6265952: MethodAccessorImpl NoClas…

a2ee859

…sDefError

gzm0 force-pushed the fix-issue-3921 branch from a414ec9 to a2ee859 Compare January 19, 2020 20:55

sjrd approved these changes Jan 19, 2020

View reviewed changes

gzm0 merged commit 5353712 into scala-js:release-v1.0.0 Jan 20, 2020

Fix #3921: Work around JDK-6265952: MethodAccessorImpl NoClassDefError #3928

Fix #3921: Work around JDK-6265952: MethodAccessorImpl NoClassDefError #3928

Uh oh!

Conversation

xerial commented Jan 16, 2020

Uh oh!

gzm0 commented Jan 17, 2020

Uh oh!

gzm0 commented Jan 17, 2020

Uh oh!

gzm0 left a comment

Choose a reason for hiding this comment

Uh oh!

gzm0 Jan 17, 2020

Choose a reason for hiding this comment

Uh oh!

gzm0 Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

sjrd commented Jan 17, 2020

Uh oh!

xerial commented Jan 17, 2020

Uh oh!

xerial commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xerial commented Jan 17, 2020

Uh oh!

gzm0 Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

xerial commented Jan 19, 2020

Uh oh!

gzm0 commented Jan 19, 2020

Uh oh!

gzm0 commented Jan 19, 2020

Uh oh!

sjrd Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

gzm0 Jan 19, 2020

Choose a reason for hiding this comment

Uh oh!

xerial commented Jan 21, 2020

Uh oh!

Uh oh!

xerial commented Jan 17, 2020 •

edited

Loading