Re: [hibernate-dev] [ORM] Synchronization on AbstractLoadPlanBasedLoader
Looks like this is related to HHH-8679 and HHH-8939:
IIRC, the code in AbstractLoadPlanBasedLoader was copied from Loader.
Here's the commit that removed the comment you mention from Loader#
wrapResultSetIfEnabled and added a synchronized block to
retreiveColumnNameToIndexCache (HHH-8679):
https://github.com/hibernate/hibernate-orm/commit/71d5a746e331d805f336585...
Here's the commit that removed the synchronized block from
Loader#retreiveColumnNameToIndexCache(HHH-8939):
https://github.com/hibernate/hibernate-orm/commit/af5804a49cc8a93a61733de...
It looks like we didn't make the same changes when we moved to load plans.
On Tue, Dec 15, 2015 at 4:49 AM, Steve Ebersole <steve(a)hibernate.org> wrote:
I did not add those comments; they were just in some code I copied
over
into that class.
On Tue, Dec 15, 2015, 4:02 AM Sanne Grinovero <sanne(a)hibernate.org> wrote:
> On 15 December 2015 at 01:46, Steve Ebersole <steve(a)hibernate.org>
wrote:
> > It's very possible that code comments may no longer be pertinent.
>
> Right, that's what I'm trying to figure out. Do you remember which
> possible deadlock it might have referred to?
>
> >
> > On Mon, Dec 14, 2015 at 10:26 AM Sanne Grinovero <sanne(a)hibernate.org>
> > wrote:
> >>
> >> Hi all,
> >> while reviewing an improvement by Stale about reducing
> >> synchronization, I'm having the impression that the synchronization
> >> could be completely removed.
> >>
> >> But there's a comment warning me against that, so for sake of safety
> >> I'm merging the improvement without risking going too far:
> >>
> >> // synchronized to avoid multi-thread access issues; defined as
> >> method synch to avoid
> >> // potential deadlock issues due to nature of code.
> >>
> >> I tried to figure what "potential deadlock" it's referring
to, but I'm
> >> having the impression the comment might be outdated. So I've reduced
> >> the contention to the only include the code block about which I'm not
> >> confident.
> >> By looking into git history, it seems the comment isn't related to any
> >> specific fix but was included already when this class was first
> >> created.
> >>
> >> Would someone be able to point out what is the issue this is
protecting
> >> against?
> >>
> >> That should allow us to provide an even better patch, although I'll
> >> apply the safe one for now so to at least have the benefits already
> >> when wrapping of result-sets is disabled.
> >>
> >> thanks,
> >> Sanne
> >> _______________________________________________
> >> hibernate-dev mailing list
> >> hibernate-dev(a)lists.jboss.org
> >> https://lists.jboss.org/mailman/listinfo/hibernate-dev
>
_______________________________________________
hibernate-dev mailing list
hibernate-dev(a)lists.jboss.org
https://lists.jboss.org/mailman/listinfo/hibernate-dev