2.31.0 C# Wrapper Crash

[JBBee]

---

Required Info
Camera Model	T265
Firmware Version	0.2.0.879
Operating System & Version	Win10
Kernel Version (Linux Only)
Platform	PC
SDK Version	2.31.0
Language	C#
Segment	AR

Issue Description

I'm experiencing a crash when using the C# wrapper in the development branch (2.31.0). The following code causes a crash:

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            using (var ctx = new Context())
            {
                var devices = ctx.QueryDevices();
                if (devices.Count == 0) return;
                var dev = devices[0]; //T265

                using (var sensor = dev.QuerySensors().First(s => s.Is(Extension.PoseSensor)))
                using (var poseSensor = PoseSensor.FromSensor(sensor)) ;
            }
            System.Console.WriteLine("Done");
        }
    }
}

There appears to be one call to NativeMethods.rs2_create_sensor in SensorList but two calls to NativeMethods.rs2_delete_sensor each when the sensor and the poseSensor above are disposed.

Previously SensorList was generic so I could call QuerySensors() with the PoseSensor type directly but this was changed in #5170

How do I now get a PoseSensor from a device?

Thanks.

[dorodnic]

Sorry for the delay, looking into it

[ogoshen]

Thanks for reporting, the crash is a bug and shouldn't happen...

The problem is the internal handle is copied from the original sensor to the pose sensor. Once a sensor is disposed, the native handle is no longer valid, and the second sensor now points to an invalid handle, so a System.AccessViolationException is thrown.

There are several possible fixes:

1. Handle is moved to the second sensor, the first is effectively disposed (trying to use it will raise an ObjectDisposedException)

2. Casting functions should return a sensor that doesn't call rs2_delete_sensor when disposed.
   Native lifetime is managed by original sensor, this can cause issues if the lifetime of original is shorter than the casted.

3. Copy the internal DeleterHandler instead of a the raw pointer, a bit like c++'s use of shared_ptr. This way disposing of either would mark all copies as disposed. That's a bigger change though...

Note that Device exhibits the same behavior, Frame has native reference count, and StreamProfile's native handle is owned by the frame, so they are not affected.

[JBBee]

Thanks for the options @ogoshen

Here's a try at number 1: cs_wrapper_fix

Let me know if it looks ok to you and I can make a pull request.

Thanks.

[ogoshen]

Looks good, Sensor.As<T> should receive similar treatment.

Though the more I think about it, seems like 3 + manual ref counting is the proper way to go.
This way the native handle would be kept alive until all users have been disposed.

The ref counter can either be a field in relevant types, or maybe better (but requires .NET>=4) use a ConditionalWeakTable like this where the keys are the native handle.

[JBBee]

Hi @ogoshen , I took another try at it following your recommendations (but without a ConditionalWeakTable). It's in this commit.

I copy references to the DeleterHandle, which now reference counts, when casting between sensors or devices.

Let me know what you think.

Thanks.

[ogoshen]

Great job!

Do have a couple of notes worth discussing:

1. Disposed ref counted objects are still completely usable.
   This is a user facing issue, and one that will probably raise some eyebrows.
   I'd expect disposing such an object would SetHandleAsInvalid, further usage should raise an ObjectDisposedException. The native handle would of course still be alive until all refs are disposed.

2. All objects are now ref counted, with refCount = 0 indicating they aren't.. (or have been disposed)
   I'm pretty cool with it, but I'd also would have liked a way to differentiate objects.
   I was thinking about composing the ref count, but maybe even just marking with an interface\trait might be enough. Anyway this is an internal impl detail and not critical.

[JBBee]

Thanks @ogoshen , my responses to your notes below.

1. Good point, this would definitely raise some eyebrows. I now point the deleter handle to an invalid handle so use of a disposed object will throw objectDisposedException.

2. I made an attempt at differentiating objects with a simple approach of extending DeleterHandle with RefCountedDeleterHandle. It's a more basic approach than the alternatives you suggested though.

The updates are in this commit

Let me know if this looks ok to you.

Thanks again.

[ogoshen]

Looks great!

Just one small issue here with:

//Reset the instance ref to an invalid handle
m_instance = new RefCountedDeleterHandle();

I'd like to avoid this allocation, the empty invalid handle could be a singleton (might also convey the meaning better), something like this should work:

public static readonly RefCountedDeleterHandle Empty;
static RefCountedDeleterHandle()
{
    Empty = new RefCountedDeleterHandle(IntPtr.Zero, null);
    GC.SuppressFinalize(Empty);
}

I'm still not 100% happy about everything looking like it's ref counted, and would have liked an extra field where needed with something like a RefCountDisposable wrapping the Object.m_instance.
This way, the object pool can go back to a single IntPtr factory, Sensor\Device can manage their own RefCountDisposable field...

But your design certainly has advantages, the biggest is the simplicity for adding new types.

You've got my 👍 for a PR

[JBBee]

Hi @ogoshen

I realized I should test the changes more thoroughly before doing a PR and when doing so wait_for_frames on a pipeline timed-out pretty quickly. Turns out I can't just set the DeleterHandle to an empty instance since when an object gets pulled from a pool stack it expects to re-use the Deleter in the DeleterHandle. So the Deleter for frames wasn't being called causing the pipeline's queue to fill up and then it timed out.

This was a fatal flaw in my design and highlights that doing managed ref-counting for all objects is unnecessary and can only cause problems. So I started over and attempted to implement what you described and adapted RefCountDisposable . I think it's much better this way. Only Sensor and Device are modified (minimally), while the frame and frameset hierarchy are left unchanged.

The re-done changes are here

Thanks for iterating on this with me, and apologies if I made this more difficult than it needed to be.

Happy new year!

Edit: I'll also include this commit in the PR to return ref-counted objects to its pool on Dispose when the ref-count hasn't yet reached zero.

Edit: I also included this commit in the PR since I realized the refcount needed to be pulled from a table indexed by the Handle

[ogoshen]

Great that you caught it in time!

My suggestion was only intuitive, I'm glad to see it had merit.
I still haven't tested it, but reviewed it and it looks much better, only relevant classes are changed and it doesn't touch the original handle.

So great that there was already a thread safe implementation for reference, getting those things right is hard.. might not have been necessary here, but better safe than sorry.

Thanks for iterating on this with me, and apologies if I made this more difficult than it needed to be.

No problem, and you haven't.
I think this is a wonderful example for open source, an exemplary issue was opened, turned into a healthy discussion, and resulted in a PR. This is what social coding is all about 🍻