Skip to content
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

"Bad constant pool" errors when running concurrently #115

Closed
szeiger opened this issue May 31, 2016 · 4 comments
Closed

"Bad constant pool" errors when running concurrently #115

szeiger opened this issue May 31, 2016 · 4 comments
Milestone

Comments

@szeiger
Copy link
Contributor

szeiger commented May 31, 2016

We're invoking MiMa from the Scala build: https://github.com/szeiger/scala/blob/3727c211806a6d462e5168372e2601dd959ce050/project/MiMa.scala. This is done by directly calling the CLI's Main.main, which in turn creates a new instance of MiMa.

When two instances run in parallel (on two different projects), they frequently fail with errors "bad constant pool errors" such as

[error] (run-main-1) java.lang.RuntimeException: bad constant pool tag 7 at byte 10
java.lang.RuntimeException: bad constant pool tag 7 at byte 10
    at com.typesafe.tools.mima.core.ClassfileParser$ConstantPool.errorBadTag(ClassfileParser.scala:204)
    at com.typesafe.tools.mima.core.ClassfileParser$ConstantPool.getName(ClassfileParser.scala:117)
    at com.typesafe.tools.mima.core.ClassfileParser.parseMember(ClassfileParser.scala:237)
    at com.typesafe.tools.mima.core.ClassfileParser$$anonfun$parseMembers$1.apply(ClassfileParser.scala:223)
    at com.typesafe.tools.mima.core.ClassfileParser$$anonfun$parseMembers$1.apply(ClassfileParser.scala:218)
    at scala.collection.immutable.Range.foreach(Range.scala:141)
    at com.typesafe.tools.mima.core.ClassfileParser.parseMembers(ClassfileParser.scala:218)
    at com.typesafe.tools.mima.core.ClassfileParser.parseMethods(ClassfileParser.scala:212)
    at com.typesafe.tools.mima.core.ClassfileParser.parseClass(ClassfileParser.scala:262)
    at com.typesafe.tools.mima.core.ClassfileParser.parseAll(ClassfileParser.scala:68)
    at com.typesafe.tools.mima.core.ClassfileParser.parse(ClassfileParser.scala:59)
    at com.typesafe.tools.mima.core.ClassInfo.ensureLoaded(ClassInfo.scala:86)
    at com.typesafe.tools.mima.core.ClassInfo.methods(ClassInfo.scala:101)
    at com.typesafe.tools.mima.core.ClassInfo$$anonfun$lookupClassMethods$2.apply(ClassInfo.scala:123)
    at com.typesafe.tools.mima.core.ClassInfo$$anonfun$lookupClassMethods$2.apply(ClassInfo.scala:123)
[...]

See https://scala-ci.typesafe.com/job/scala-2.11.x-validate-test/1978/console for a complete log.

This does not happen when only once instance of MiMa runs at a time.

szeiger added a commit to szeiger/scala that referenced this issue May 31, 2016
@dwijnand
Copy link
Collaborator

I wonder if concurrency is also the cause of #112

szeiger added a commit to szeiger/scala that referenced this issue Jun 14, 2016
- Support directories in `-doc-external-doc`: It is documented as
  accepting a “classpath_entry_path” for the keys but this only worked
  for JARs and not for individual class files. When checking for
  external-doc mappings for a Symbol, we now find the root directory
  relative to a class file instead of using the full class file path.
  The corresponding tests for SI-191 and SI8557 are also fixed to
  support individual class files instead of JARs in partest. This is
  required for the sbt build which runs partest on “quick” instead of
  “pack”.

- Fix version and repository handling for bootstrapping. The bootstrap
  `scalaInstance` can now be resolved from any repository added to the
  project (not just the bootstrap repositories) by using a different
  workaround for sbt/sbt#1872.

- Workaround for sbt/sbt#2640 (putting the
  wrong `scalaInstance` on partest’s classpath). The required
  `ScalaInstance` constructor is deprecated, so we have to disable
  deprecation warnings and fatal warnings until there is a better fix.

- Add MiMa to the sbt build (port of the old `test.bc` ant task). The
  sbt-mima plugin doesn’t have all the features we need, so we do it
  manually in a similar way to what the plugin does. Checks are done
  in both directions for the `library` and `compiler` projects. The
  base version has to be set in `build.sbt`. When set to `None`, MiMa
  checks are skipped. MiMa checks are run sequentially to avoid spurious
  errors (see lightbend-labs/mima#115).

- Port the OSGi tests to the sbt build. The set of JARs that gets copied
  into build/osgi as bundles is a bit different from the ant build. We
  omit the source JARs but add additional modules that are part of the
  Scala distribution, which seems more correct.

- Get rid up `pull-binary-libs.sh` for the sbt build. Add artifacts are
  resolved from the special bootstrap repository through Ivy. The
  special `code.jar` and `instrumented.jar` artifacts are copied to the
  location where partest expects them (because these paths are hardcoded
  in partest). Other extra JARs for partest in `test/files/lib` are
  referenced directly from the Ivy cache.

- Move common settings that should be available with unqualified names
  in local `.sbt` files and on the command line into an auto-plugin.

- Add an `antStyle` setting to sbt to allow users to easily enable
  ant-style incremental compilation instead of sbt’s standard name
  hashing with `set antStyle := true`.

- Disable verbose `info`-level logging during sbt startup for both,
  `validate/test` and `validate/publish-core` jobs. Update logging is
  no longer disabled when running locally (where it is useful and
  doesn’t generate excessive output).

- Pass optimization flags for scalac down to partest, using the new
  partest version 1.0.15.

- Call the new sbt-based PR validation from `scripts/jobs/validate/test`.
szeiger added a commit to szeiger/scala that referenced this issue Jun 14, 2016
- Support directories in `-doc-external-doc`: It is documented as
  accepting a “classpath_entry_path” for the keys but this only worked
  for JARs and not for individual class files. When checking for
  external-doc mappings for a Symbol, we now find the root directory
  relative to a class file instead of using the full class file path.
  The corresponding tests for SI-191 and SI8557 are also fixed to
  support individual class files instead of JARs in partest. This is
  required for the sbt build which runs partest on “quick” instead of
  “pack”.

- Fix version and repository handling for bootstrapping. The bootstrap
  `scalaInstance` can now be resolved from any repository added to the
  project (not just the bootstrap repositories) by using a different
  workaround for sbt/sbt#1872.

- Workaround for sbt/sbt#2640 (putting the
  wrong `scalaInstance` on partest’s classpath). The required
  `ScalaInstance` constructor is deprecated, so we have to disable
  deprecation warnings and fatal warnings until there is a better fix.

- Add MiMa to the sbt build (port of the old `test.bc` ant task). The
  sbt-mima plugin doesn’t have all the features we need, so we do it
  manually in a similar way to what the plugin does. Checks are done
  in both directions for the `library` and `compiler` projects. The
  base version has to be set in `build.sbt`. When set to `None`, MiMa
  checks are skipped. MiMa checks are run sequentially to avoid spurious
  errors (see lightbend-labs/mima#115).

- Port the OSGi tests to the sbt build. The set of JARs that gets copied
  into build/osgi as bundles is a bit different from the ant build. We
  omit the source JARs but add additional modules that are part of the
  Scala distribution, which seems more correct.

- Get rid up `pull-binary-libs.sh` for the sbt build. Add artifacts are
  resolved from the special bootstrap repository through Ivy. The
  special `code.jar` and `instrumented.jar` artifacts are copied to the
  location where partest expects them (because these paths are hardcoded
  in partest). Other extra JARs for partest in `test/files/lib` are
  referenced directly from the Ivy cache.

- Move common settings that should be available with unqualified names
  in local `.sbt` files and on the command line into an auto-plugin.

- Add an `antStyle` setting to sbt to allow users to easily enable
  ant-style incremental compilation instead of sbt’s standard name
  hashing with `set antStyle := true`.

- Disable verbose `info`-level logging during sbt startup for both,
  `validate/test` and `validate/publish-core` jobs. Update logging is
  no longer disabled when running locally (where it is useful and
  doesn’t generate excessive output).

- Pass optimization flags for scalac down to partest, using the new
  partest version 1.0.15\6.

- Call the new sbt-based PR validation from `scripts/jobs/validate/test`.

- Disable the tests `run/t7843-jsr223-service` and `run/t7933` from
  scala#4959 for now. We need to set up
  a new test project (either partest or junit) that can run them on a
  packaged version of Scala, or possibly move them into a separate
  project that would naturally run from a packaged Scala as part of the
  community build.
@dotta
Copy link
Contributor

dotta commented Jun 28, 2016

I guess one possible workaround could be to use separate classloaders.

milessabin pushed a commit to typelevel/scala that referenced this issue Aug 17, 2016
- Support directories in `-doc-external-doc`: It is documented as
  accepting a “classpath_entry_path” for the keys but this only worked
  for JARs and not for individual class files. When checking for
  external-doc mappings for a Symbol, we now find the root directory
  relative to a class file instead of using the full class file path.
  The corresponding tests for SI-191 and SI8557 are also fixed to
  support individual class files instead of JARs in partest. This is
  required for the sbt build which runs partest on “quick” instead of
  “pack”.

- Fix version and repository handling for bootstrapping. The bootstrap
  `scalaInstance` can now be resolved from any repository added to the
  project (not just the bootstrap repositories) by using a different
  workaround for sbt/sbt#1872.

- Workaround for sbt/sbt#2640 (putting the
  wrong `scalaInstance` on partest’s classpath). The required
  `ScalaInstance` constructor is deprecated, so we have to disable
  deprecation warnings and fatal warnings until there is a better fix.

- Add MiMa to the sbt build (port of the old `test.bc` ant task). The
  sbt-mima plugin doesn’t have all the features we need, so we do it
  manually in a similar way to what the plugin does. Checks are done
  in both directions for the `library` and `compiler` projects. The
  base version has to be set in `build.sbt`. When set to `None`, MiMa
  checks are skipped. MiMa checks are run sequentially to avoid spurious
  errors (see lightbend-labs/mima#115).

- Port the OSGi tests to the sbt build. The set of JARs that gets copied
  into build/osgi as bundles is a bit different from the ant build. We
  omit the source JARs but add additional modules that are part of the
  Scala distribution, which seems more correct.

- Get rid up `pull-binary-libs.sh` for the sbt build. Add artifacts are
  resolved from the special bootstrap repository through Ivy. The
  special `code.jar` and `instrumented.jar` artifacts are copied to the
  location where partest expects them (because these paths are hardcoded
  in partest). Other extra JARs for partest in `test/files/lib` are
  referenced directly from the Ivy cache.

- Move common settings that should be available with unqualified names
  in local `.sbt` files and on the command line into an auto-plugin.

- Add an `antStyle` setting to sbt to allow users to easily enable
  ant-style incremental compilation instead of sbt’s standard name
  hashing with `set antStyle := true`.

- Disable verbose `info`-level logging during sbt startup for both,
  `validate/test` and `validate/publish-core` jobs. Update logging is
  no longer disabled when running locally (where it is useful and
  doesn’t generate excessive output).

- Pass optimization flags for scalac down to partest, using the new
  partest version 1.0.15\6.

- Call the new sbt-based PR validation from `scripts/jobs/validate/test`.

- Disable the tests `run/t7843-jsr223-service` and `run/t7933` from
  scala#4959 for now. We need to set up
  a new test project (either partest or junit) that can run them on a
  packaged version of Scala, or possibly move them into a separate
  project that would naturally run from a packaged Scala as part of the
  community build.
@JoshRosen
Copy link
Contributor

JoshRosen commented Oct 6, 2016

I managed to make a bit of headway in investigating this and think that I may have spotted one potential source of thread-unsafety.

I added some additional logging in ClassFileParser to print more details when an error occurs:

diff --git a/core/src/main/scala/com/typesafe/tools/mima/core/ClassfileParser.scala b/core/src/main/scala/com/typesafe/tools/mima/core/ClassfileParser.scala
index fb8c960..d18760c 100644
--- a/core/src/main/scala/com/typesafe/tools/mima/core/ClassfileParser.scala
+++ b/core/src/main/scala/com/typesafe/tools/mima/core/ClassfileParser.scala
@@ -58,7 +58,8 @@ abstract class ClassfileParser(definitions: Definitions) {
     try {
       parseAll(clazz)
     } catch {
-      case e: RuntimeException => handleError(e)
+      case e: RuntimeException =>
+        handleError(new RuntimeException("Got exception while processing " + clazz.file.name + " with " + this, e))
     }
   }

When running this on Spark's build, I managed to hit the following failure:

[error] (mllib-local/*:mimaFindBinaryIssues) Got exception while processing Object.class with com.typesafe.tools.mima.core.LibClassfileParser@3812664d
[error] (launcher/*:mimaFindBinaryIssues) Got exception while processing Object.class with com.typesafe.tools.mima.core.LibClassfileParser@3812664d

As you can see from these logs, it looks like the same LibClassFileParser instance is being used in both builds and instances of ClassfileParser are not thread-safe because they contain unsynchronized mutable state.

A common pattern in the failures that I've seen is that they all seem to involve Object.class. For example, another failure that I've seen in concurrent runs is

[error] (launcher/*:mimaFindBinaryIssues) java.io.IOException: class file '/Library/Java/JavaVirtualMachines/jdk1.8.0_66.jdk/Contents/Home/jre/lib/rt.jar(java/lang/Object.class)' has wrong magic number 0xfebabe00, should be 0xcafebabe

It looks like there's a global ClassInfo.ObjectClass object, which, in turn, references the global Config.baseDefinitions, which has a ClassFileParser. This ClassInfo.ObjectClass can be referenced as a superclass when parsing other ClassInfo objects. After adding a bit of additional logging around the construction of this global ClassInfo.ObjectClass object, I managed to trigger the failure again and it seems that this is indeed is the source of the problematic shared ClassFileParser.

It seems like a simple fix might be to just add synchronized to ClassFileParser.parse, which appears to be the only method that's called from outside of that class itself.

@SethTisue
Copy link
Collaborator

optimistically closing on the assumption #129 fixed it

@SethTisue SethTisue added this to the 0.1.11 milestone Oct 19, 2016
ghost pushed a commit to dbtsai/spark that referenced this issue Oct 21, 2016
We should upgrade to the latest release of MiMa (0.1.11) in order to include a fix for a bug which led to flakiness in the MiMa checks (lightbend-labs/mima#115).

Author: Josh Rosen <[email protected]>

Closes apache#15571 from JoshRosen/SPARK-18034.
asfgit pushed a commit to apache/spark that referenced this issue Oct 21, 2016
We should upgrade to the latest release of MiMa (0.1.11) in order to include a fix for a bug which led to flakiness in the MiMa checks (lightbend-labs/mima#115).

Author: Josh Rosen <[email protected]>

Closes #15571 from JoshRosen/SPARK-18034.
robert3005 pushed a commit to palantir/spark that referenced this issue Nov 1, 2016
We should upgrade to the latest release of MiMa (0.1.11) in order to include a fix for a bug which led to flakiness in the MiMa checks (lightbend-labs/mima#115).

Author: Josh Rosen <[email protected]>

Closes apache#15571 from JoshRosen/SPARK-18034.
uzadude pushed a commit to uzadude/spark that referenced this issue Jan 27, 2017
We should upgrade to the latest release of MiMa (0.1.11) in order to include a fix for a bug which led to flakiness in the MiMa checks (lightbend-labs/mima#115).

Author: Josh Rosen <[email protected]>

Closes apache#15571 from JoshRosen/SPARK-18034.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

5 participants