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

scala.reflect.runtime.ThreadLocalStorage$MyThreadLocalStorage is forever retaining references to threads #8946

Closed
scabug opened this issue Oct 29, 2014 · 16 comments
Assignees
Milestone

Comments

@scabug
Copy link

scabug commented Oct 29, 2014

I'm having an issue in one of my applications where it is crashing about once every week or so due to a slow, steady memory leak. For brief overview of a recent heap dump in VisualVM, please watch this brief video I've recorded: http://screencast.com/t/3qiNuzwa

After doing my best to reason about the problem, and still admittedly rather confused about the issue, this is the best hypothesis I can form:

I did my best to capture a stack-trace of when these thread references are being inserted into the ThreadLocalStorage:

promotion-api scala.reflect.runtime.ThreadLocalStorage$MyThreadLocalStorage.set(ThreadLocalStorage.scala:30)
promotion-api scala.reflect.runtime.SynchronizedTypes$class.subsametypeRecursions_$eq(SynchronizedTypes.scala:55)
promotion-api scala.reflect.runtime.JavaUniverse.subsametypeRecursions_$eq(JavaUniverse.scala:16)
promotion-api scala.reflect.internal.tpe.TypeComparers$class.isSubType(TypeComparers.scala:284)
promotion-api scala.reflect.internal.SymbolTable.isSubType(SymbolTable.scala:16)
promotion-api scala.reflect.internal.Types$Type.$less$colon$less(Types.scala:779)
promotion-api scala.reflect.internal.Types$Type.$less$colon$less(Types.scala:260)
promotion-api scaldi.TypeTagIdentifier.sameAs(Identifier.scala:46)
promotion-api scaldi.Identifiable$$anonfun$isDefinedFor$2$$anonfun$apply$4.apply(Binding.scala:10)
promotion-api scaldi.Identifiable$$anonfun$isDefinedFor$2$$anonfun$apply$4.apply(Binding.scala:10)
promotion-api scala.collection.LinearSeqOptimized$class.exists(LinearSeqOptimized.scala:79)
promotion-api scala.collection.immutable.List.exists(List.scala:83)
promotion-api scaldi.Identifiable$$anonfun$isDefinedFor$2.apply(Binding.scala:10)
promotion-api scaldi.Identifiable$$anonfun$isDefinedFor$2.apply(Binding.scala:10)
promotion-api scala.collection.LinearSeqOptimized$class.forall(LinearSeqOptimized.scala:69)
promotion-api scala.collection.immutable.List.forall(List.scala:83)
promotion-api scaldi.Identifiable$class.isDefinedFor(Binding.scala:10)
promotion-api scaldi.LazyBinding.isDefinedFor(Binding.scala:65)
promotion-api scaldi.Module$$anonfun$getBindingInternal$1.apply(Module.scala:24)
promotion-api scaldi.Module$$anonfun$getBindingInternal$1.apply(Module.scala:24)
promotion-api scala.collection.LinearSeqOptimized$class.find(LinearSeqOptimized.scala:99)
promotion-api scala.collection.immutable.List.find(List.scala:83)
promotion-api scaldi.Module$class.getBindingInternal(Module.scala:24)
promotion-api com.spingo.promotion.Global$$anon$1.getBindingInternal(Boot.scala:83)
promotion-api scaldi.InjectorWithLifecycle$$anonfun$getBinding$3.apply(Injector.scala:138)
promotion-api scaldi.InjectorWithLifecycle$$anonfun$getBinding$3.apply(Injector.scala:138)
promotion-api scaldi.util.Util$WorkflowHelper$.$bar$greater$extension(Util.scala:7)
promotion-api scaldi.InjectorWithLifecycle$class.getBinding(Injector.scala:138)
promotion-api com.spingo.promotion.Global$$anon$1.getBinding(Boot.scala:83)
promotion-api scaldi.Injectable$class.scaldi$Injectable$$injectWithDefault(Injectable.scala:43)
promotion-api scaldi.Injectable$$anonfun$inject$1.apply(Injectable.scala:22)
promotion-api scaldi.Injectable$$anonfun$inject$1.apply(Injectable.scala:22)
promotion-api scaldi.util.Util$WorkflowHelper$.$bar$greater$extension(Util.scala:7)
promotion-api scaldi.Injectable$class.inject(Injectable.scala:22)
promotion-api scaldi.Injectable$.scaldi$OpenInjectable$$super$inject(Injectable.scala:91)
promotion-api scaldi.OpenInjectable$class.inject(Injectable.scala:73)
promotion-api scaldi.Injectable$.inject(Injectable.scala:91)
promotion-api com.spingo.promotion.actors.HttpActor.<init>(HttpActor.scala:53)
promotion-api com.spingo.promotion.ActorBindings$$anonfun$26$$anonfun$apply$2.apply(Boot.scala:66)
promotion-api com.spingo.promotion.ActorBindings$$anonfun$26$$anonfun$apply$2.apply(Boot.scala:66)
promotion-api akka.actor.TypedCreatorFunctionConsumer.produce(Props.scala:343)
promotion-api akka.actor.Props.newActor(Props.scala:252)
promotion-api akka.actor.ActorCell.newActor(ActorCell.scala:552)
promotion-api akka.actor.ActorCell.create(ActorCell.scala:578)
promotion-api akka.actor.ActorCell.invokeAll$1(ActorCell.scala:456)
promotion-api akka.actor.ActorCell.systemInvoke(ActorCell.scala:478)
promotion-api akka.dispatch.Mailbox.processAllSystemMessages(Mailbox.scala:263)
promotion-api akka.dispatch.Mailbox.run(Mailbox.scala:219)
promotion-api akka.dispatch.ForkJoinExecutorConfigurator$AkkaForkJoinTask.exec(AbstractDispatcher.scala:393)
promotion-api scala.concurrent.forkjoin.ForkJoinTask.doExec(ForkJoinTask.java:260)
promotion-api scala.concurrent.forkjoin.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1339)
promotion-api scala.concurrent.forkjoin.ForkJoinPool.runWorker(ForkJoinPool.java:1979)
promotion-api scala.concurrent.forkjoin.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:107)

(Line numbers for ThreadLocalStorage.scala are off because I'd added some code to help with debugging)

It seems that ThreadLocalStorage should use a storage mechanism that allows for garbage collection of the keys, such as WeakHashMap.

I have pushed a suggested fix to github, here:

https://github.com/spingo/scala/tree/issue/SI-8946

I am open to suggestions on how to write an effective test for this.

@scabug
Copy link
Author

scabug commented Oct 29, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8946?orig=1
Reporter: @timcharper
Affected Versions: 2.11.0, 2.11.1, 2.11.2

@scabug
Copy link
Author

scabug commented Oct 29, 2014

@timcharper said:
I have signed the Scala contributor license agreement (found here: https://www.typesafe.com/contribute/cla/scala/sign)

@scabug
Copy link
Author

scabug commented Oct 29, 2014

@timcharper said (edited on Oct 29, 2014 7:05:39 AM UTC):
Looks like my patch causes the tests to fail:

     [echo] Checking backward binary compatibility for scala-reflect (against 2.11.0)
     [mima] Found 1 binary incompatibiities (27 were filtered out)
     [mima] ======================================================
     [mima]  * method values()java.util.concurrent.ConcurrentHashMap in class
     [mima]    scala.reflect.runtime.ThreadLocalStorage#MyThreadLocalStorage has now a
     [mima]    different result type; was: java.util.concurrent.ConcurrentHashMap, is now:
     [mima]    java.util.Map
     [mima] Generated filter config definition
     [mima] ==================================
     [mima]
     [mima]     filter {
     [mima]         problems=[
     [mima]             {
     [mima]                 matchName="scala.reflect.runtime.ThreadLocalStorage#MyThreadLocalStorage.values"
     [mima]                 problemName=IncompatibleResultTypeProblem
     [mima]             }
     [mima]         ]
     [mima]     }
     [mima]

I'm not sure why we care about binary compatibility for a private class. I will look to see if there is an efficient way to use ConcurrentHashMap with weak keys.

@scabug
Copy link
Author

scabug commented Oct 29, 2014

@timcharper said:
With your confirmation, and given this is a private class whose instances are all directly inaccessible to the outside world, I'll add the above to the bincompat whitelist.

Also, I think the value of MyThreadLocalStorage should be marked private, too. What are your thoughts?

@scabug
Copy link
Author

scabug commented Nov 1, 2014

@timcharper said:
I will work today to add regression tests for this bug, then send a pull-request. If you have any guidance, or are already working on a solution, please let me know so I don't repeat efforts.

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@retronym said:
Thank's for the patch, Tim. I have this ticket on my TODO list for this week, sorry I haven't had a chance to comment yet. Would be great if you can find a reliable way to regression test this! I would put this sort of test under ./test/files/run, just define an object Test in the file with a main method. You can run it via the partest harness with ./test/partest test/files/run/t8946.scala.

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@retronym said:
Here's a test that seems to reliably show the problem:

object Test {
  def main(args: Array[String]): Unit = {
    var i = 0 

    while (i < 16) {
      i += 1
      val t = new Thread { 
        val ballast = Array.ofDim[Byte](256 * 1024 * 1024)
        override def run(): Unit = {
          import reflect.runtime.universe._
          typeOf[List[String]] <:< typeOf[Seq[_]]
  t.start()
  t.join()
}

}
}
{code}

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@timcharper said:
Great work; Was working at it from a much less effective angle. This is genius. So, we can run this a few times, add weak refs to it, force gc, and check to make sure the weak refs are cleared?

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@retronym said (edited on Nov 3, 2014 11:07:26 AM UTC):
I think it is enough for that test not to run out of heap; it will retain 4G at the moment. I suppose we can increase 16 to 64 to be really sure. GC will happen if we approach a full heap, no need to trigger explicitly.

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@timcharper said (edited on Nov 3, 2014 11:56:09 AM UTC):
This works reliably without having to rely on GC collection on heap exhaustion:

{code:title=MemTest.scala}
package mem_test

import scala.ref.WeakReference

object Main extends App {
def forceGc() = {
var obj = new Object
val ref = new WeakReference(obj)
obj = null;
while(ref.get.nonEmpty)
System.gc()
}

val threads = for (i <- (1 to 16)) yield {
val t = new Thread {
override def run(): Unit = {
import reflect.runtime.universe._
typeOf[List[String]] <:< typeOf[Seq[]]
}
}
t.start()
t.join()
WeakReference(t)
}
forceGc()
val nonGCdThreads = threads.map(
.get).flatten.length
assert(nonGCdThreads == 0, s"${nonGCdThreads} threads were retained; expected 0.")
}
{code}

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@timcharper said:
I'm able to confirm that this fails with Scala 2.11.2, and passes with my applied patch:

https://github.com/spingo/scala/tree/issue/SI-8946

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@retronym said:
AFAIK JVM implementations are free to ignore System.gc() so arguably the brute force approach is more portable.

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@timcharper said:
I've pushed that test to the patch.

Are we okay to whitelist the binary incompatible change? That's the final item, I think, before this pull request is ready to go.

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@timcharper said:
Jason - okay - I'll have it allocate a bunch of memory until the weak-ref goes rather than calling System.gc

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@timcharper said:
Patch changed; please have a look when you get a second: SpinGo/scala@issue/SI-8946

Thanks for your help with this!

@scabug
Copy link
Author

scabug commented Nov 3, 2014

@retronym said (edited on Nov 3, 2014 12:40:21 PM UTC):
I think the whitelist is okay here. The rules in Migration Manager (MiMa) are based on Java access; scalac has to widen certain Scala-private things to JVM-public.

Could you please submit a pull request against the 2.11.x branch of scala/scala and @-mention me (retronym) and Eugene (xeno-by) as a reviewers?

@scabug scabug closed this as completed Nov 26, 2014
@scabug scabug added this to the 2.11.5 milestone Apr 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants