Improve mapping manager synchronization#772
Conversation
|
@avivcarmis would you mind rebasing this on top of 3.x? Otherwise I can take care of it. |
| mappers.putIfAbsent(klass, mapper); | ||
| } | ||
| return mapper; | ||
| return (Mapper<T>) mappers.get(klass); |
There was a problem hiding this comment.
I might be nitpicking but I think the following would be still more efficient:
Mapper<T> mapper = (Mapper<T>) mappers.get(klass);
if (mapper == null) {
EntityMapper<T> entityMapper = ...;
mapper = new Mapper<T>(this, klass, entityMapper);
Mapper<T> old = (Mapper<T>) mappers.putIfAbsent(klass, mapper);
if (old != null)
mapper = old;
}
return mapper;... because in the nominal case (i.e. there is already an entry in the map) there would be just one access to the map instead of two.
There was a problem hiding this comment.
Yeah, i'll change it.
I'm actually re-thinking that whole PR, these entire parts (1, 2, etc...) may not work.
I think the safest tweak will be to go back to the original implementation and add
final Object lock = new Object() for each of those maps and synchronize them when constructing. Less pretty, but less error prone i guess.
What do you think?
There was a problem hiding this comment.
@avivcarmis onTableRemoved and onTableChanged should work on a best-effort basis. Honestly, if people out there are crazy enough to alter their schema while clients are issuing requests, it's their problem. I would actually suggest that we get rid of the synchronized blocks in the places you spotted and clearly specify in the documentation that concurrent modifications of the schema could end up in unexpected results.
|
Done. |
OK so following this discussion, I've used
ConcurrentHashMap::putIfAbsentto prevent an illegal state where two different instances of same mapper are created and returned.Since this issue is non-deterministic (depends on execution timing to be reproduced) i don't really have a way to test it.
There are two issues with my implementation.
ConcurrentHashMap'scomputeIfAbsent, which means that prior to the first put call there might be some redundant creations of instances, the good news is, they will never be returned to the user so at least the original issue is solved.SchemaChangeListenerBaseis now not synchronized with creation of instances, this means that there can be a scenario where a table is removed and the error message is not logged.To solve both issues, we can revert back to regular
HashMap, previous implementation, but create a dedicated final lock for each map, this way we get all we need. Down side is we implement synchronization ourselves, which means we probably get lower performance and might have concurrency issues. (Same as earlier actually)Let me know what you think...