Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Object.Finalize should not be proxiable #3205

Closed
ssharunas opened this issue Nov 28, 2022 · 1 comment · Fixed by #3233
Closed

Object.Finalize should not be proxiable #3205

ssharunas opened this issue Nov 28, 2022 · 1 comment · Fixed by #3233

Comments

@ssharunas
Copy link

Currently in DefaultDynamicProxyMethodCheckerExtensions methods IsProxiable and ShouldBeProxiable are treating overridden Finalize methods as proxiable:

...
&& (method.DeclaringType != typeof(object) || !"finalize".Equals(method.Name, StringComparison.OrdinalIgnoreCase))
...

Both of these methods are used for lazy loading.
If during finalization object is not yet loaded, lazy proxy attempts to pull it from DB.
Since Finalize is called by GC at random times and random threads, it is unlikely that session will still be open during Finalize.
In most cases Finalize for lazy not yet loaded proxies throws an exception "Could not initialize proxy - no Session".

This bug occurs only if proxyable class has a base class, that is overriding Finalize, ie:

class Base { ~Base() { } }
class LazyChild : Base { } // this class will not be finalized

I've created a small demonstration project: https://github.com/ssharunas/nhibernate-finalization-bug-demonstration

IMHO Finalize should not be proxyable even in delivered classes.

@hazzik
Copy link
Member

hazzik commented Nov 29, 2022

Removed my earlier comment. This seems legit.

@hazzik hazzik changed the title Object.Finalize should not be proxiable Object.Finalize should not be proxiable Feb 3, 2023
bahusoid added a commit to hazzik/nhibernate-core that referenced this issue Mar 22, 2023
@hazzik hazzik added this to the 5.5 milestone Apr 3, 2023
@hazzik hazzik added the r: Fixed label Apr 3, 2023
hazzik added a commit to hazzik/nhibernate-core that referenced this issue Apr 3, 2023
hazzik added a commit that referenced this issue Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants