Intermittent runtime StackOverflowError with multi-level sealed hierarchy

[einholen]

Hi! I'm intermittently getting a StackOverflowError at runtime because the Magnolia macro resolves itself implicitly instead of the actual concrete type class.
I saw #66 and #190 but they seem to be different, so creating a new issue. Feel free to close if it's a dupe and unresolvable in Scala 2 🙂

So there's a sealed trait with a bunch of sealed subtraits each of which has a bunch of concrete case classes underneath. We use magnolia to dispatch specific type classes based on an instance of the sealed trait at runtime. We don't use combine, only dispatch. All branches of the hierarchy terminate, and each concrete class has an implicit defined that Magnolia is supposed to find.

With a hierarchy like this

sealed trait Base
sealed trait Sub1 extends Base { def param: Int }
case class Concrete1(param: Int) extends Sub1
sealed trait Sub2 extends Base { def param: String }
case class Concrete2(param: String) extends Sub2

and implicits for concrete case classes defined in a separate compilation unit from the Magnolia macro

object Show {
  type Typeclass[T] = Show[T]
  def combine[T <: Base](caseClass: CaseClass[Show, T]): Show[T] = throw new UnsupportedOperationException
  def dispatch[T <: Base](sealedTrait: SealedTrait[Show, T]): Show[T] = new Show[T] {
    def show(value: T): String = sealedTrait.dispatch(value) { subtype =>
      subtype.typeclass.show(subtype.cast(value))
    }
  }
  implicit def gen[T <: Base]: Show[T] = macro Magnolia.gen[T]
}

object Implicits {
  implicit val showConcrete1: Show[Concrete1] = (c: Concrete1) => s"Concrete1:${c.param}"
  implicit val showConcrete2: Show[Concrete2] = (c: Concrete2) => s"Concrete2:${c.param}"
}

Calling implicitly[Show[Base]].show(Concrete2("hello")) sometimes results in a runtime StackOverflowError.

After a clean compilation the error is gone, but changes to adjacent code seem to sometimes resurrect it.
What I'm seeing in debug inside sealedTrait.dispatch is, subtypesArray contains Concrete1 and Sub2. A value of Concrete2 thus matches Sub2, and then subtype.typeclass resolves to Show.gen again and again instead of recursing inside the sealed trait and finding the actual concrete implicit.
After a clean compilation, subtypesArray does not have any traits and properly has all case classes.

I could partially solve the problem by making the Show.gen non-implicit and calling it directly where needed - apparently without itself available as a "catch-all" implicit it understands that it needs to go deeper into the sealed hierarchy, and lands into dispatch twice: for Sub2 and then for Concrete2.

Scastie workbook with the full code: https://scastie.scala-lang.org/Hou8fzlAR0anAPeUHvWpFw
I couldn't reproduce the problem in Scastie - It works as intended, most likely because everything is in one file. We can emulate a runtime stackOverflow in this workbook, however, if we remove one of the concrete implicits (it won't be exactly the case I'm describing above but it would follow the same implicit resolution patterns as in my intermittent problem).

[propensive]

Thanks for the detailed writeup, @einholen! There is a certain amount global mutable state involved in running Magnolia, which is needed to persist some information between different invocations of the macro within the same compilation unit, but which unfortunately seems to persist between different invocations of the compiler. If there's more than one concurrent instance of the compiler handling compilations, then it can be even harder to diagnose.

But it does seem like we're still carrying over some state between compilations which we shouldn't be. I'm a little surprised that the outcome isn't a compiler crash. That would be better than a runtime error, frankly...

[einholen]

Ah I didn't know Magnolia (or macros in general) can carry over state between compilations. I think it makes sense. What I didn't mention because I thought it's irrelevant - we use Hydra in the project to speed up compilation by parallelizing it, so it probably contributes to the issue. I thought a clean compilation always fixes it but with this extra context from you I now think that it sometimes fixes it. We also don't use Hydra on CI and that could be why this problem never occurred in prod and I've only ever seen it when running unit tests locally.

[joroKr21]

Oh yeah, expanding Magnolia macros in parallel is not well defined.
Perhaps it would be good to put a disclaimer in the README about Magnolia and Hydra,
but we need some way to check that this is in fact what is actually happening,
i.e. Magnlia macros being expanded in parallel.

[joroKr21]

Another option would be to try and use ThreadLocal although I'm not sure if Hydra is using raw threads or Futures (ThreadLocal does not work with Future). @einholen do you have a clue?

[joroKr21]

@propensive perhaps we should contact Triplequote to get a free licence for Magnolia and add it to our CI and try to reproduce the problem. WDYT?

[joroKr21]

@einholen in the meantime you can try out #217 - you can build and publish Magnolia locally to do that.
Let me know if you need help with that.

[joroKr21]

@einholen can you try the latest release?

[ghost]

Posting a link to an IntelliJ specific issue with magnolia for posterity. The Scala compile server is backed by a thread pool which reuses long-running threads (whose thread local state never gets cleared), and having magnolia in the project seems to retain a large heap and could lead to OOM errors.

https://youtrack.jetbrains.com/issue/SCL-20909/Scala-compiler-server-memory-leak.#focus=Comments-27-6763912.0-0

I'm open to have a broader discussion about magnolia and the Scala compile server.

cc @joroKr21

[joroKr21]

It might be as simple as clearing the local state at the end of macro expansion.

[ghost]

I'm curious to try that out, from a practical standpoint, but I'm obviously not suggesting that any changes need to be done to the project, especially since I don't know whether that can be done safely and not have an adverse effect on the compilation, both in correctness and/or performance.

On the other hand, I also don't particularly feel like making some changes, or adding exceptions on the IntelliJ side, at least not before we have a bit more knowledge on the matter.

In any case, thanks for taking a look.

[adamw]

@joroKr21 it seems the thread locals are cleaned ...

magnolia/core/src/main/scala/magnolia1/magnolia.scala

Lines 899 to 909 in 2a5d7ec

	def withContext(c: whitebox.Context)(fn: (Stack[c.type], Int) => c.Tree): c.Tree = {
	val stack = threadLocalStack.get()
	val workSet = threadLocalWorkSet.get()
	workSet += c.macroApplication.symbol
	val depth = c.enclosingMacros.count(m => workSet(m.macroApplication.symbol))
	try fn(stack.asInstanceOf[Stack[c.type]], depth)
	finally if (depth <= 1) {
	stack.clear()
	workSet.clear()
	}
	}

so it's probably sth more subtle than that :)

[joroKr21]

Right I forgot about it. Maybe we also need to null out the thread locals themselves? But I doubt it.

[Caparow]

@joroKr21 removing thread locals instead of just cleaning Stack and Set seems to have helped to resolve the issue. I've tested this with a project which uses Magnolia a lot.

May we release a new version with these changes?

  object Stack {
    // Cheating to satisfy Singleton bound (which improves type inference).
    private val dummyContext: whitebox.Context = null
    private val threadLocalStack = ThreadLocal.withInitial[Stack[dummyContext.type]](() => new Stack[dummyContext.type])
    private val threadLocalWorkSet = ThreadLocal.withInitial[mutable.Set[whitebox.Context#Symbol]](() => mutable.Set.empty)

    def withContext(c: whitebox.Context)(fn: (Stack[c.type], Int) => c.Tree): c.Tree = {
      val stack = threadLocalStack.get()
      val workSet = threadLocalWorkSet.get()
      workSet += c.macroApplication.symbol
      val depth = c.enclosingMacros.count(m => workSet(m.macroApplication.symbol))
      try fn(stack.asInstanceOf[Stack[c.type]], depth)
      finally if (depth <= 1) {
        threadLocalStack.remove()
        threadLocalWorkSet.remove()
      }
    }
  }

[adamw]

@Caparow can you maybe create a PR with this change? will be easiest to review

[Caparow]

@adamw I've created a PR #448