Synchronize reflection code as scala 2.10 reflection is not threadsafe #16

Closed
timcharper opened this Issue Apr 7, 2014 · 5 comments

Comments

Projects
None yet
2 participants
@timcharper

Hi Oleg,

Many thanks for this fine library! It's serving me well. And, sorry that my gratitude comes with an issue report (but at least it has a solution :) ).

I am encountering a thread-safety issue when instantiating binding Modules in scala-test. As you may know, scala-test likes to run tests in parallel. Here is my sample code:

package com.spingo.promotion.db.queries

import com.spingo.test.helpers.{ RolledbackDbSession, ScopedFixtures, Workbench }
import com.spingo.promotion.helpers.SpingoSpec
import com.spingo.promotion.DatabaseBindings
import com.github.nscala_time.time.Imports._
import scaldi._
import scaldi.Injectable._

class AvailabilityQueriesSpec extends SpingoSpec with ScopedFixtures with RolledbackDbSession {
  // ...
  val bindingModule = LazyFixture {
    new Module with DatabaseBindings {}
  }

where DatabaseBindings is a trait, containing my DAL bindings:

trait DatabaseBindings extends Module {
  bind [DateTime] identifiedBy 'now toProvider DateTime.now
  bind [AvailabilityQueries] to new AvailabilityQueries
  // ...
}

(LazyFixture is a library that I wrote for scala-test. It provides a unique instance of some object for test case, in lazy fashion.)

This is the error I am receiving:

[info]       scala.reflect.internal.Symbols$CyclicReference: illegal cyclic reference involving anonymous class $$anonfun$4
[info]       at scala.reflect.internal.Symbols$Symbol$$anonfun$info$3.apply(Symbols.scala:1218)
[info]       at scala.reflect.internal.Symbols$Symbol$$anonfun$info$3.apply(Symbols.scala:1216)
[info]       at scala.Function0$class.apply$mcV$sp(Function0.scala:40)
[info]       at scala.runtime.AbstractFunction0.apply$mcV$sp(AbstractFunction0.scala:12)
[info]       at scala.reflect.internal.Symbols$Symbol.lock(Symbols.scala:482)
[info]       at scala.reflect.internal.Symbols$Symbol.info(Symbols.scala:1216)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.lookup(JavaMirrors.scala:821)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.scala$reflect$runtime$JavaMirrors$JavaMirror$$constructorToScala1(JavaMirrors.scala:856)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$constructorToScala$1.apply(JavaMirrors.scala:852)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$constructorToScala$1.apply(JavaMirrors.scala:852)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$toScala$1.apply(JavaMirrors.scala:104)
[info]       at scala.reflect.runtime.TwoWayCache.toScala(TwoWayCache.scala:38)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.toScala(JavaMirrors.scala:102)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.constructorToScala(JavaMirrors.scala:852)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.sOwner(JavaMirrors.scala:780)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.scala$reflect$runtime$JavaMirrors$JavaMirror$$classToScala1(JavaMirrors.scala:942)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$classToScala$1.apply(JavaMirrors.scala:935)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$classToScala$1.apply(JavaMirrors.scala:935)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$toScala$1.apply(JavaMirrors.scala:104)
[info]       at scala.reflect.runtime.TwoWayCache.toScala(TwoWayCache.scala:38)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.toScala(JavaMirrors.scala:102)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.classToScala(JavaMirrors.scala:935)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.typeToScala(JavaMirrors.scala:1036)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.scala$reflect$runtime$JavaMirrors$JavaMirror$$targToScala$1(JavaMirrors.scala:1023)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$targsToScala$1.apply(JavaMirrors.scala:1025)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$targsToScala$1.apply(JavaMirrors.scala:1025)
[info]       at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
[info]       at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
[info]       at scala.collection.immutable.List.foreach(List.scala:318)
[info]       at scala.collection.TraversableLike$class.map(TraversableLike.scala:244)
[info]       at scala.collection.AbstractTraversable.map(Traversable.scala:105)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.targsToScala(JavaMirrors.scala:1025)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.typeToScala(JavaMirrors.scala:1044)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$FromJavaClassCompleter.completeRest(JavaMirrors.scala:679)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$FromJavaClassCompleter$LazyPolyType.complete(JavaMirrors.scala:727)
[info]       at scala.reflect.internal.Symbols$Symbol.info(Symbols.scala:1229)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.lookup(JavaMirrors.scala:821)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.scala$reflect$runtime$JavaMirrors$JavaMirror$$constructorToScala1(JavaMirrors.scala:856)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$constructorToScala$1.apply(JavaMirrors.scala:852)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$constructorToScala$1.apply(JavaMirrors.scala:852)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$toScala$1.apply(JavaMirrors.scala:104)
[info]       at scala.reflect.runtime.TwoWayCache.toScala(TwoWayCache.scala:38)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.toScala(JavaMirrors.scala:102)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.constructorToScala(JavaMirrors.scala:852)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.sOwner(JavaMirrors.scala:780)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.scala$reflect$runtime$JavaMirrors$JavaMirror$$classToScala1(JavaMirrors.scala:942)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$classToScala$1.apply(JavaMirrors.scala:935)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$classToScala$1.apply(JavaMirrors.scala:935)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$toScala$1.apply(JavaMirrors.scala:104)
[info]       at scala.reflect.runtime.TwoWayCache.toScala(TwoWayCache.scala:38)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.toScala(JavaMirrors.scala:102)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.classToScala(JavaMirrors.scala:935)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.typeToScala(JavaMirrors.scala:1036)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.scala$reflect$runtime$JavaMirrors$JavaMirror$$targToScala$1(JavaMirrors.scala:1023)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$targsToScala$1.apply(JavaMirrors.scala:1025)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$targsToScala$1.apply(JavaMirrors.scala:1025)
[info]       at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
[info]       at scala.collection.TraversableLike$$anonfun$map$1.apply(TraversableLike.scala:244)
[info]       at scala.collection.immutable.List.foreach(List.scala:318)
[info]       at scala.collection.TraversableLike$class.map(TraversableLike.scala:244)
[info]       at scala.collection.AbstractTraversable.map(Traversable.scala:105)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.targsToScala(JavaMirrors.scala:1025)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.typeToScala(JavaMirrors.scala:1044)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$FromJavaClassCompleter.completeRest(JavaMirrors.scala:679)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$FromJavaClassCompleter.complete(JavaMirrors.scala:667)
[info]       at scala.reflect.internal.Symbols$Symbol.info(Symbols.scala:1229)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.lookup(JavaMirrors.scala:821)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.scala$reflect$runtime$JavaMirrors$JavaMirror$$constructorToScala1(JavaMirrors.scala:856)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$constructorToScala$1.apply(JavaMirrors.scala:852)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$constructorToScala$1.apply(JavaMirrors.scala:852)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$toScala$1.apply(JavaMirrors.scala:104)
[info]       at scala.reflect.runtime.TwoWayCache.toScala(TwoWayCache.scala:38)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.toScala(JavaMirrors.scala:102)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.constructorToScala(JavaMirrors.scala:852)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.sOwner(JavaMirrors.scala:780)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.scala$reflect$runtime$JavaMirrors$JavaMirror$$classToScala1(JavaMirrors.scala:942)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$classToScala$1.apply(JavaMirrors.scala:935)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$classToScala$1.apply(JavaMirrors.scala:935)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror$$anonfun$toScala$1.apply(JavaMirrors.scala:104)
[info]       at scala.reflect.runtime.TwoWayCache.toScala(TwoWayCache.scala:38)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.toScala(JavaMirrors.scala:102)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.classToScala(JavaMirrors.scala:935)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.classSymbol(JavaMirrors.scala:206)
[info]       at scala.reflect.runtime.JavaMirrors$JavaMirror.classSymbol(JavaMirrors.scala:65)
[info]       at scaldi.ReflectionBinder$class.reflectiveBindings(Binder.scala:129)
[info]       at com.spingo.promotion.db.queries.AvailabilityQueriesSpec$$anonfun$4$$anon$1.reflectiveBindings$lzycompute(AvailabilityQueriesSpec.scala:25)

My issue is solved by wrapping the instantiation of the Module with synchronized { ... }, as such:

  val bindingModule = LazyFixture {
    synchronized {
      new Module with DatabaseBindings {}
    }
  }

I think using a global synchronization wrapper around https://github.com/scaldi/scaldi/blob/master/src/main/scala/scaldi/Binder.scala#L123 will solve the issue.

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Apr 7, 2014

I've cloned the library, tried a few things, and wasn't able to come up with a solution (by changing the library). Apparently, the only thing that works is to run synchronized on the reference to test-suite itself. I'm not really sure how this would be fixed.

I've cloned the library, tried a few things, and wasn't able to come up with a solution (by changing the library). Apparently, the only thing that works is to run synchronized on the reference to test-suite itself. I'm not really sure how this would be fixed.

@OlegIlyenko OlegIlyenko added this to the 0.4 milestone Apr 7, 2014

@OlegIlyenko OlegIlyenko self-assigned this Apr 7, 2014

@OlegIlyenko OlegIlyenko added the bug label Apr 7, 2014

@OlegIlyenko OlegIlyenko modified the milestones: 0.3.1, 0.4 Apr 7, 2014

@OlegIlyenko

This comment has been minimized.

Show comment
Hide comment
@OlegIlyenko

OlegIlyenko Apr 7, 2014

Member

Hi Tim, thanks a lot for the detailed problem description! It's actually me who needs to apologize. I knew that scala's reflection is not thread-safe, and still haven't secured the only place where it really used in scaldi.

I will try to find a good solution for this issue asap (looks like a decent puzzle, I like these :). Meanwhile, I think that it was a mistake to mix ReflectionBinder to the WordBinder in Module. Even in your example you are using only WordBinder DSL and still you face the issue that is only relevant for ReflectionBinder. But in general I think it would be better to keep these two concepts separate from each other. So I probably will make StaticModule the only injector that support reflective bindings.

i will keep you updated

Member

OlegIlyenko commented Apr 7, 2014

Hi Tim, thanks a lot for the detailed problem description! It's actually me who needs to apologize. I knew that scala's reflection is not thread-safe, and still haven't secured the only place where it really used in scaldi.

I will try to find a good solution for this issue asap (looks like a decent puzzle, I like these :). Meanwhile, I think that it was a mistake to mix ReflectionBinder to the WordBinder in Module. Even in your example you are using only WordBinder DSL and still you face the issue that is only relevant for ReflectionBinder. But in general I think it would be better to keep these two concepts separate from each other. So I probably will make StaticModule the only injector that support reflective bindings.

i will keep you updated

@OlegIlyenko

This comment has been minimized.

Show comment
Hide comment
@OlegIlyenko

OlegIlyenko Apr 7, 2014

Member

BTW, just reproduced the error successfully (was pretty easy :). I see all kinds of exceptions, including the one you've faced.

I also checked out Scala 2.11.0-RC4 and every single problem is just gone :) Looks like they've done a great job on SI-6240. Now I need to think about the strategy how to deal with this issue for now and in future (when 2.11 would be released).

Member

OlegIlyenko commented Apr 7, 2014

BTW, just reproduced the error successfully (was pretty easy :). I see all kinds of exceptions, including the one you've faced.

I also checked out Scala 2.11.0-RC4 and every single problem is just gone :) Looks like they've done a great job on SI-6240. Now I need to think about the strategy how to deal with this issue for now and in future (when 2.11 would be released).

@OlegIlyenko

This comment has been minimized.

Show comment
Hide comment
@OlegIlyenko

OlegIlyenko Apr 24, 2014

Member

Now that scala 2.11 officially released, I also released new version of scaldi with it's support. Would it be possible for you to update to the latest scala/scaldi version? (this should resolve issues that we discussed here)

Member

OlegIlyenko commented Apr 24, 2014

Now that scala 2.11 officially released, I also released new version of scaldi with it's support. Would it be possible for you to update to the latest scala/scaldi version? (this should resolve issues that we discussed here)

@timcharper

This comment has been minimized.

Show comment
Hide comment
@timcharper

timcharper Apr 25, 2014

Exciting! Yes, we’ll be upgrading to Scala 2.11 soon. Thank you!

On Apr 24, 2014, at 14:04, Oleg Ilyenko notifications@github.com wrote:

Now that scala 2.11 officially released, I also released new version of scaldi with it's support. Would it be possible for you to update to the latest scala/scaldi version? (this should resolve issues that we discussed here)


Reply to this email directly or view it on GitHub.

Exciting! Yes, we’ll be upgrading to Scala 2.11 soon. Thank you!

On Apr 24, 2014, at 14:04, Oleg Ilyenko notifications@github.com wrote:

Now that scala 2.11 officially released, I also released new version of scaldi with it's support. Would it be possible for you to update to the latest scala/scaldi version? (this should resolve issues that we discussed here)


Reply to this email directly or view it on GitHub.

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