hibernate-orm
hibernate-orm copied to clipboard
hibernate
HHH 14873 - PostFlush / PreFlush and lazy loading lead to ConcurrentModificationException
See issue description at https://hibernate.atlassian.net/browse/HHH-14873
You may step back one commit (revert the solution) to see that the newly added test really fails.
@beikov this may be interesting to you, judging from the stuff from me you reviewed before
@sebersole mentioned in the discussion on Jira, that the PR would not be accepted with the current implementation. Therefore I would like to have a discussion about how to fix that flaw in order to deliver an acceptable solution. Please provice some feedback.
First a question about how a non-fixed version is supposed to work: If a user of Hibernate uses the pre- and postFlush() methods to iterate over entities, what would be the best way to avoid lazy-loading entities?
Now about the tradeoffs in the current implementation in this PR:
The only places where the managedEntitiesIterator is used are the Interceptor callbacks preFlush() and postFlush(). There is, however, the potential that some developers using Hibernate access the managedEntitiesIterator through the PersistenceContext interface or through the StatefulPersistenceContext class, where the method is marked public.
The implementation proposed currently might impact performance by duplicating the number of pointers to managed entities when the iterator is accessed. This takes linear time and memory over the number of managed entities.
Other possible implementations:
There are countless possibilities to implement a fix, ranging from introducing additional callback variants to just documenting the pitfall for users of the API.
Another possibility is not to use the managedEntitiesIterator for preFlush() and postFlush(). That way, it would only impact performance in situations where the ConcurrentModificationException is most likely. All other potential uses of the public managedEntitiesIterator() method would not be impacted.
Add a setting which opts in to this behavior. Easy.
You mean a setting that is read by the EntityManagerFactoryBuilderImpl and the SessionFactoryOptionsBuilder to then generate EntityManager / Session instances that have that setting, which then can be read in managedEntitiesIterator() to decide whether to copy the entities on demand or not?
Copying the iterator on access is now optional and a setting (hibernate.safe_managed_entities_iterator) and false by default, to avoid affecting any existing users.
I just fixed the checkstyle warning. @sebersole I wondered, if it is possible to use this line instead of my custom iterator:
return Arrays.stream(entityEntryContext.reentrantSafeEntityEntries()).map(Entry::getKey).iterator();
I am not sure whether the entity entries in entityEntryContext contain the same entities as the entitiesByKey. If they do, this would probably be the more "standard" way and we could even think about doing this unconditionally (see my comment on the Jira issue for explanation)