Fix #415 - initialization of pickler registry when switching runtime strategies #416
Conversation
jvican
changed the title
|
Shall we merge this @jsuereth? |
|
Sorry for delay, a few comments coming. |
|
No worries, I do understand that you're busy 😄. Thanks for your time reviewing this! |
| */ | ||
| def replaceRuntime(r: PicklingRuntime): Unit = { | ||
| if(r.picklers.isLookupEnabled) { | ||
| currentRuntime.picklers.dumpStateTo(r.picklers) |
There was a problem hiding this comment.
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.
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 PicklingProtocol before using it. I forgot to explain that, my apologies. However, we cannot rely on the user to do this, since even if the pickling protocol is imported in scope, it won't be preallocated. One has to explicitly pull something from the protocol, like implicitly[Pickler[List[Int]]]. Then, it gets initialized and the runtime changes.
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 synchronized? I'd do it just to shape a more robust api and make explicit the fact that it's not thread-safe. Anyway, as the user will rarely use it, the synchronized overhead will be minimal.
There was a problem hiding this comment.
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.
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 StaticOnly, I'll open an issue to explain what I mean by this).
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.
I agree :) Looking forward to readi gthe bug.
|
Ok, not ready to merge, but I like the direction. In terms of registering picklers again from maps, there are solutions to that which I can show you if needed. As I said before, best to avoid existential types until you need them, generally. |
|
This is again for review 😄 |
| type PicklerGenerator = FastTypeTag[_] => Pickler[_] | ||
| type UnpicklerGenerator = FastTypeTag[_] => Unpickler[_] | ||
|
|
||
| type PicklerGen = FastTypeTag[_] => Pickler[_] |
There was a problem hiding this comment.
I see here, I was inconsistent in my existential usage (reality, I think I was just moving code around and never cleaned it up :) ).
runtime allows lookups. The state is composed by the registered
picklers and unpicklers.
/cc @jsuereth