Random segfaults caused by trying to dispose a `null` `SkSvgImage` object in `SvgImage` class finalizer about questpdf HOT 4 CLOSED

Thank you for reporting this issue! It is quite an interesting case! Apparently, the object finalizer is called even when the exception is thrown in its constructor.

It should be fixed in the 2024.3.3 release. Would you please give it a try?

from questpdf.

Hi @MarcinZiabek,

thank you again for your prompt response and resolution (as usual).

I confirm the segfault is not happening anymore with 2024.3.3 release using your fix (as already mentioned in the initial report, I did exactly the same quick test before coming to you with this issue).

Please allow me to make a small comment regarding this approach, more like a pedantic note with respect to the nullable type checking (when <Nullable>enable</Nullable>): SkSvgImage property is not declared as nullable but it is checked for null later on in the finalizer.

On the other hand...

It is quite an interesting case! Apparently, the object finalizer is called even when the exception is thrown in its constructor.

...it is called indeed, here is my opinion on it (I'm sorry if my English skill fails to deliver): the catch is that we have a managed object wrapping a managed object wrapping an unmanaged resource (the native Instance handle from SkSvgImage class). :-) Unfortunately this whole situation doesn't look safe, the finalizers will be called later on by a dedicated "GC cleaning" thread, and the SvgImage finalizer will execute anyway because the "top" managed object was actually created right after its new constructor completed, even if its wrapped resources are not properly constructed / created (SkSvgImage and its wrapped native Instance created by SkSvgImage.API.svg_create() failed), so the SkSvgImage property is null already from the SvgImage constructor (even if the actual SvgImage object is not null).

from questpdf.

Please allow me to make a small comment regarding this approach, more like a pedantic note with respect to the nullable type checking (when enable): SkSvgImage property is not declared as nullable but it is checked for null later on in the finalizer.

Generally speaking, I agree entirely that nullable types could be improved in the library. I made a couple of attempts to improve things, but many times, introducing all null-checks only complicated code without introducing much value. As far as I can observe, TypeScript or Kotlin are much more clever at determining nullability and producing warnings. C# is not meeting my expectations; henceforth, I tend not to always follow this pattern.

...it is called indeed, here is my opinion on it (I'm sorry if my English skill fails to deliver): the catch is that we have a managed object wrapping a managed object wrapping an unmanaged resource (the native Instance handle from SkSvgImage class). :-) Unfortunately this whole situation doesn't look safe, the finalizers will be called later on by a dedicated "GC cleaning" thread, and the SvgImage finalizer will execute anyway because the "top" managed object was actually created right after its new constructor completed, even if its wrapped resources are not properly constructed / created (SkSvgImage and its wrapped native Instance created by SkSvgImage.API.svg_create() failed), so the SkSvgImage property is null already from the SvgImage constructor (even if the actual SvgImage object is not null).

So far, I think that as long as we check if a given unmanaged resource has been instantiated before disposing it, everything should be fine. The SvgImage does not implement IDisposable by design. I wanted to avoid a similar situation as with HttpClient, where it is not clear whether the object should be disposed. In QuestPDF, the lifetime of SvgImage is also not obvious, as the library uses lambda functions and deferred execution, and therefore it can be tricky to programmatically dispose this resource.

The time will tell whether I am wrong only a little bit or entirely 😆

from questpdf.

Hello @MarcinZiabek,

thank you for your feedback.

[...] but many times, introducing all null-checks only complicated code without introducing much value [...]

I can totally understand that. :-)
Support for nullable references was introduced in C# years ago, it was only a few months since I decided to actually give it a try in my projects. When I initially investigated it I drew exactly the same conclusion as you did. In the end, after forcing myself to use it in a few projects I can say that if we can get past the initial feeling, it starts to grow on us little by little and overall even the slightest help the compiler tries to gives us with this feature makes us a bit less prone to certain class of errors.

So far, I think that as long as we check if a given unmanaged resource has been instantiated before disposing it, everything should be fine.

In this particular case I also think it is not a problem, but still, in general, the recommended dispose pattern should be followed, to avoid possible unforeseeable consequences.

The SvgImage does not implement IDisposable by design. I wanted to avoid a similar situation as with HttpClient, where it is not clear whether the object should be disposed. In QuestPDF, the lifetime of SvgImage is also not obvious, as the library uses lambda functions and deferred execution, and therefore it can be tricky to programmatically dispose this resource.

Absolutely, that is exactly why we rely on finalizers to do their job and we like (or dislike) GC languages. :-) It is not about doing it programmatically, but rather to ensure that when the GC decides to kick in and collect the object, the finalizer call does not produce any issues.
The tricky part here is the way C# works when constructing the object wrapping a native resource and when the constructor throws and still later on the GC calls its finalizer (and this is by design), so the finalizer should be ready to handle construction failures (with respect to the object's unmanaged resources). I tried to find a useful resource on the internet about this topic and my search engine gave me this: https://eranstiller.com/net-finalize-and-constructor-exceptions -- in my opinion it is a short and good read for any .NET developer and explains it better than my previous poor attempt.

The time will tell whether I am wrong only a little bit or entirely 😆

Oh, please don't worry about it, and please don't consider at all my comment as a critique. Most probably I have less experience with .NET than you do. In my opinion it should never be about being wrong or not, it should be about what we learn from each experience, and from my personal experience :-) usually we learn more from "wrong" than otherwise, but for sure we learn a lot by exchanging ideas with each other (and that was my only reasoning behind the comment).

Anyway, getting back to the topic, I want to thank you again for taking your time developing this library and kindly responding to my comments; I double confirm the issue is not happening anymore and I think it is safe to close this issue as solved.

from questpdf.