Guards against use of a disposed session factory #3120
Conversation
| CheckNotDisposed(); | ||
| return eventListeners; |
There was a problem hiding this comment.
The session factory closing Destroy the event listeners, so better guard against any later use of them.
| CheckNotDisposed(); | ||
| return new SessionBuilderImpl(this); |
There was a problem hiding this comment.
I have mostly protected accesses to resources destroyed or disposed by the session factory closing, except here and in stateless equivalent, where there is no direct accesses to these resources. But that is the main usage of session factories, so, better to directly throw here when using it on a disposed factory.
| CheckNotDisposed(); | ||
| IEntityPersister value; | ||
| if (entityPersisters.TryGetValue(entityName, out value) == false) |
There was a problem hiding this comment.
entityPersisters have their cache property destroyed by the factory closing, so guarding against their use. This also causes most session usages to throw if their session factory is disposed, which is also the aim of this PR.
| CheckNotDisposed(); | ||
| ICollectionPersister value; | ||
| if (collectionPersisters.TryGetValue(role, out value) == false) |
There was a problem hiding this comment.
Same here, their cache is destroyed by the factory closing.
| CheckNotDisposed(); | ||
| return | ||
| _allCacheRegions |
There was a problem hiding this comment.
Caches are destroyed by the closing.
| CheckNotDisposed(); | ||
| _allCacheRegions.TryGetValue(regionName, out var result); | ||
| return result; |
There was a problem hiding this comment.
_allCacheRegions is also accessed by private GetCache, which I do not guard because it is currently called only by already directly guarded methods or upon initialization of the session factory.
| if (queryCache != null) | ||
| CheckNotDisposed(); |
There was a problem hiding this comment.
Not sure I should have that if. Done for consistency with EvictQueries.
| private void CheckNotDisposed() | ||
| { | ||
| if (disposed) | ||
| { | ||
| throw new ObjectDisposedException($"Session factory {Name} with id {Uuid}"); | ||
| } | ||
| } |
There was a problem hiding this comment.
I am going to change this to CheckNotClosed, because we can close a factory without disposing it, and closing it is enough to cause the reported troubles.
Help diagnose nhibernate#3026 complains
fredericDelaporte
force-pushed
the
GH3026
branch
from
4dc6413 to
06510f8
Compare
|
I had to force the MyGet publishing on TeamCity. Maybe I should have just waited, but I was not seeing any reason for it to have not run after the TeamCity "Release" build was finished. |
Help diagnose #3026 reports.
I suspect those reports are due to user code errors, using a disposed session factory. It appears a disposed session factory does not throw when used. It lets things fail quite later with harder to understand exceptions. This leads to bug reports for what is likely user bugs.
This PR currently targets 5.3.x. But this may be considered as too much of a possible breaking change for this branch, even if using a disposed session factory is a bug on user side.
In my opinion it is ok to have it in 5.3.x.