-
Notifications
You must be signed in to change notification settings - Fork 79
Fix #415 - initialization of pickler registry when switching runtime strategies #416
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,13 +5,12 @@ import java.util.concurrent.atomic.AtomicReference | |
import scala.language.experimental.macros | ||
import scala.language.reflectiveCalls | ||
|
||
import java.util.IdentityHashMap | ||
|
||
import HasCompat._ | ||
|
||
package object internal { | ||
|
||
import scala.reflect.runtime.{universe => ru} | ||
import spi._ | ||
import ru._ | ||
import compat._ | ||
|
||
|
@@ -24,10 +23,19 @@ package object internal { | |
|
||
} | ||
} | ||
private[this] var currentRuntimeVar = new AtomicReference[spi.PicklingRuntime](initDefaultRuntime) | ||
def currentRuntime: spi.PicklingRuntime = currentRuntimeVar.get | ||
// Here we inject a new runtime for usage. | ||
def replaceRuntime(r: spi.PicklingRuntime): Unit = { | ||
private[this] var currentRuntimeVar = new AtomicReference[PicklingRuntime](initDefaultRuntime) | ||
def currentRuntime: PicklingRuntime = currentRuntimeVar.get | ||
|
||
/** Replace the old [[PicklingRuntime]] keeping its state. This operation | ||
* is not thread-safe and it's expected to be executed in a single thread. | ||
* | ||
* Note that we don't do anything with the [[RefRegistry]] because in | ||
* future versions we are going to change how cyclic references work. | ||
*/ | ||
def replaceRuntime(r: PicklingRuntime): Unit = { | ||
if(r.picklers.isLookupEnabled) { | ||
currentRuntime.picklers.dumpStateTo(r.picklers) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A quick note: This is not thread safe. It may be ok to assume that we'll only ever swap runtimes on one thread, but if the issue is somehow the runtime is being created on one thread and replaced on another, this won't work. I think in the event there are multiple instantiation threads and this function hasn't completed first, we're already screwed, so it's probably ok as is, we just need to heavily document how to safely override the runtime and instantiate picklers. E.g. the documentation for how to repalce runtime right now shows how to create a new object to contain your entire "protocol". That's part 1 to making sure this is safe. Part #2 is to make sure you pre-allocate that object in your "main" thread before you spawn sub-threads. I had originally punted a bit on this problem, but given we're adding this functionality (which does need to be there), we should really document this problem and possible user solutions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, I didn't even thought about thread safety. As you have correctly pointed out, the real problem is that we're not pre-allocating the object Since we cannot rely on the user always doing that, I implemented this ugly solution. Yet it is the only one that will work no matter what the user decides to do. We should probably work more on the thread safety. Do you think it would be a good idea to mark this method as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue with synchronizing this method is you'd need to ALSO synchronize all other access to the pickler registry, whcih slows down general runtime. We already use thread-safe lookup (in the eventual consistency version of thread safety), so I'd hate to furtehr slow down lookup with a full up synchronize. There are ways to force things to initialize without forcing implicitly, specifically if you use the "cake" approach for your default picklers. It's not pretty, but it is workable. In any case, as i said, I think what you have is fine, we just need to document WHY we think it's fine and where it is not fine :). As long as it's documented, users may be surprised, but able to cope. that's better than surprised and have no idea how to resolve the issue. The best case would be if we had a legitimate solution to the problem. Given the current architecture, I think we do not, but I'm happy to hear alternative ideas :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that this is good for a short-term solution, but we should think about a new "cake" approach, because the way to customize the runtime is not sleek. Especially if this runtime strategy depends on implicits that the user may put into scope (like But, atm, I agree that we should stick with this and greatly document it. I do not see any other alternative solution. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree :) Looking forward to readi gthe bug. |
||
} | ||
currentRuntimeVar.lazySet(r) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
package scala.pickling.registry | ||
|
||
import org.scalatest.FunSuite | ||
|
||
import scala.pickling._ | ||
import scala.pickling.internal.HybridRuntime | ||
import scala.pickling.json.JsonFormats | ||
import scala.pickling.pickler.AllPicklers | ||
|
||
object Protocol extends { | ||
val oldRuntime = internal.currentRuntime | ||
val currentRuntime = new HybridRuntime | ||
val onlyLookup = internal.replaceRuntime(currentRuntime) | ||
} with Ops with AllPicklers with JsonFormats | ||
|
||
class SwitchRuntimeRegistryInit extends FunSuite { | ||
|
||
import Protocol._ | ||
|
||
test("registry should be initialized when switching runtime strategies") { | ||
|
||
case class Foo(i: Int) | ||
val pf = implicitly[AbstractPicklerUnpickler[Foo]] | ||
|
||
// If the test passes, this should not initialize | ||
// the registry again. If it fails it does. | ||
implicitly[Pickler[List[String]]] | ||
|
||
try { | ||
val lookup = currentRuntime.picklers.lookupPickler(pf.tag.key) | ||
assert(lookup !== None) | ||
|
||
val pf2 = implicitly[AbstractPicklerUnpickler[Foo]] | ||
val lookup2 = currentRuntime.picklers.lookupPickler(pf.tag.key) | ||
assert(lookup === lookup2) | ||
} finally { | ||
internal.replaceRuntime(oldRuntime) | ||
// Just in case it doesn't get init | ||
implicitly[Pickler[List[String]]] | ||
} | ||
|
||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see here, I was inconsistent in my existential usage (reality, I think I was just moving code around and never cleaned it up :) ).