6
\$\begingroup\$

I need something akin to C++'s shared_ptr. Essentially I need an IDisposable item that I can ensure is cleaned up as soon as possible once all references have been removed, but this resource will be shared between many classes.

I don't allow passing in of a an already existing object as it's an easy way to know that the item I'm using isn't being hasn't already been disposed.

Unfortunately I can't think of a way to prevent the user of Item from disposing the underlying item, but as this is to go into my own program I'm not too concerned about that because it intuitively makes sense to not dispose something within a wrapper, provided every class that needs the item uses the wrapper and not just the contained item.

I simply want to know if anyone can see any potential issues with this solution besides what's mentioned.

public class SharedDisposable<T> : IDisposable where T : class, IDisposable {
    static SharedDisposable(){
        instances = new Dictionary<T, List<SharedDisposable<T>>>();
    }

    public SharedDisposable(SharedDisposable<T> share) {
        Item = share.Item;
        instances[Item].Add(this);
    }
    public SharedDisposable(object[] ctorArgs = null) {
        if(ctorArgs == null)
            ctorArgs = new object[] { };

        var ctor = typeof(SharedDisposable<T>).GetConstructor(ctorArgs.Select(a => a.GetType()).ToArray());
        Item = (T)ctor.Invoke(ctorArgs);
        instances[Item] = new List<SharedDisposable<T>>() { this };
    }
    public SharedDisposable(KeyValuePair<Type, object>[] ctorArgs) {
        var ctor = typeof(SharedDisposable<T>).GetConstructor(ctorArgs.Select(a => a.Key).ToArray());
        Item = (T)ctor.Invoke(ctorArgs.Select(a => a.Value).ToArray());
        instances[Item] = new List<SharedDisposable<T>>() { this };
    }

    public void Dispose() {
        var instanceList = instances[Item];
        instanceList.Remove(this);
        if(instances.Count == 0) {
            instances.Remove(Item);
            Item.Dispose();
        }
    }

    public T Item { get; }
    static Dictionary<T, List<SharedDisposable<T>>> instances;
}

Example usage:

class MainForm : Form {
    public MainForm(){
        InitializeComponent();

        Port = new SharedDisposable<SerialPort>("COM5", 9600);
    }
    public MainForm(SharedDisposable<SerialPort> share){
        InitializeComponent();

        if(share == null)
            throw new ArgumentNullException(nameof(share))
        Port = new SharedDisposable<SerialPort>(share);
    }

    public void BtnDevice1Info_Click(object sender, EventArgs e){
        new Device1InfoForm(Port).Show();
    }
    public void BtnDevice2Info_Click(object sender, EventArgs e){
        new Device2InfoForm(Port).Show();
    }

    override Dispose(bool disposing){
        if(disposing){
            Port.Dispose();
        }
        base.Dispose(disposing);
    }

    private SharedDisposable<SerialPort> Port { get; }
}

In my application I actually have a Modbus interface instead of serial port but I just realised the way the constructor is setup won't allow me to use the Modbus API because the Modbus API uses factories instead of constructors, so I'll need to adjust it for that. But that API should be able to queue messages to be sent out one at a time, preventing any conflicts on the serial port while also allowing multiple forms or even applications to use the same serial port. In order to make this happen I need multiple forms to be able to share a disposable resource, but none of them to clean it up unless they're the last holder of that resource.

\$\endgroup\$
  • \$\begingroup\$ Could you add an example that shows how this is supposed to be used? \$\endgroup\$ – t3chb0t Jul 16 '18 at 15:36
  • 1
    \$\begingroup\$ @t3chb0t Updated to include usage example. Here it would be possible to close MainForm and have the other forms remain open with access to the resource. \$\endgroup\$ – TheBeardedQuack Jul 16 '18 at 16:15
  • \$\begingroup\$ I'm not sure about it... maybe it'll work for other forms but not for the MainForm. Closing it exits the application. \$\endgroup\$ – t3chb0t Jul 16 '18 at 18:07
  • \$\begingroup\$ @t3chb0t: Depends on the main loop. Application.Run(new MainForm()) will exit once the MainForm gets closed. Application.Run() or a manual loop with Application.DoEvents() can keep running even when the MainForm gets closed. \$\endgroup\$ – hoffmale Jul 17 '18 at 10:36
6
\$\begingroup\$

Adding to t3chb0t's answer...

Thread safety

The current solution isn't thread safe, as access to instances isn't synchronized. Maybe use a lock or a thread-safe dictionary class instead?

In C++, std::shared_ptr ensures that updates to the control block ("ref count") are thread safe (atomic). This means creating copies or destroying an instance of a std::shared_ptr doesn't corrupt the control block, i.e. can safely be made in a concurrent environment.

Memory usage

It isn't really necessay to store a List<SharedDisposable<T>> in instances: A simple counter would be enough. This reduces the memory footprint by quite a lot.

Design

Is instances as a static field really necessary? There are other design possibilities that don't have to rely on a static field, and are more extensible if you later realize that you need a std::weak_ptr equivalent.

For example, the counter (and surrounding logic) could be kept inside a ControlBlock (to use the C++ standardese name) class. Then, an instance of ControlBlock gets created with the first SharedDisposable reference, and every subsequent copy gets a reference to it. No need to manage pseudo-global state!

However, this would eliminate the (easy) possibility of iterating over all SharedDisposable<T> for a given T.

\$\endgroup\$
  • \$\begingroup\$ Just had a thought that using a static dictionary - perhaps with the value being just a counter though - would allow the SharedDisposable class to recognise duplicate elements and add to the same counter. If someone did var share1 = new SharedDisposable<SerialPort>(() => new SerialPort()); and later passed the port into a function that then also does var share2 = new SharedDisposable<SerialPort>(() => port); then it could add on to the same counter. Whereas without the dictionary it wouldn't have any way of finding the key. Just a thought, I'm not too concerned about this scenario though \$\endgroup\$ – TheBeardedQuack Jul 18 '18 at 7:41
  • \$\begingroup\$ @TheBeardedQuack: It depends whether you want to support that behavior. But if you do, beware that this means there might be other direct references to the SerialPort instance, which will get issues if they outlast the last SharedDisposable<SerialPort> instance which were supposed to be the only owners of that SerialPort instance. I think this will cause more problems than it solves. \$\endgroup\$ – hoffmale Jul 18 '18 at 10:39
8
\$\begingroup\$
public SharedDisposable(object[] ctorArgs = null)
{
  if (ctorArgs == null)
      ctorArgs = new object[] { };

  var ctor = typeof(SharedDisposable<T>).GetConstructor(ctorArgs.Select(a => a.GetType()).ToArray());
  Item = (T)ctor.Invoke(ctorArgs);
  instances[Item] = new List<SharedDisposable<T>>() { this };
}

This constructor is not a very good idea because the use needs to exactly know the signature of the constructor in order to create T.

A much better way would be to let him specify a factory lambda. Then all it will be left is:

public SharedDisposable(Func<T> create)
{
    Item = create();
    instances[Item] = new List<SharedDisposable<T>>() { this };
}

so now he can call it in a much easier fashion like:

Port = new SharedDisposable<SerialPort>(() => new SerialPort("COM5", 9600));

The other constructor using KeyValuePairs is now obsolete.


public SharedDisposable(object[] ctorArgs = null)
{
  if (ctorArgs == null)
      ctorArgs = new object[] { };
  ...
}

Btw. you can make this look cleaner with the coalesce operator: ??

ctorArgs = ctorArgs ?? new object[] { };

Objects like this one usully use the name Value for what you chose Item.


Its usage might be easier in certain scenarios if you added an implicit cast operator to T:

public static implicit operator T(SharedDisposable<T> share) => share.Value;

If you replaced the List by a HashSet the Dispose method could be simplified to:

public void Dispose()
{
    if (instances[Value].Remove(this) && instances[Value].Count == 0)
    {
        instances.Remove(Value);
        Value.Dispose();
    }
}
\$\endgroup\$
  • 3
    \$\begingroup\$ A factory lambda makes sense for something like Lazy<T>, but here you're invoking it immediately and only once, so why not just pass in the object directly? \$\endgroup\$ – Pieter Witvoet Jul 17 '18 at 10:01
  • \$\begingroup\$ @PieterWitvoet this is a good point ;-] \$\endgroup\$ – t3chb0t Jul 17 '18 at 10:03
  • \$\begingroup\$ The lambda can hint that you shouldn't just pass in an existing instance, but it doesn't prevent it. Does anyone know of anyway to specifically check that the instance does not exist before the SharedDisposable constructor is called? I suppose one very poor way would be to call the lambda twice and check that it returns a difference instance (indicating that a new object has been created, but again not guaranteeing it), then immediately dispose of one. \$\endgroup\$ – TheBeardedQuack Jul 17 '18 at 15:44
3
\$\begingroup\$

Revision 2

Taking into account all your feedback, this is what I have now.

  • Replaced the constructors with a single constructor that accepts a function for creating the instance. This will allow constructor or factories to be used, and also avoids any issues with type-mismatching in Reflection.
  • Added locks to everywhere I believe is necessary to provide thread safe functionality.
  • Changed the static dictionary of T and instances, for a class containing a the value and a counter, which will be referenced from shared copies.
  • Added a .Share() function as a synonym for the constructor, to be used in using.

    public class SharedDisposable<T> : IDisposable where T : class, IDisposable {
        private class Target {
            public T value;
            public uint count;
        }
    
        // Lambda constructor, because reflection is too strict.
        // This will also help with classes that use factories.
        public SharedDisposable(Func<T> create) {
            target = new Target() {
                value = create(),
                count = 1,
            };
        }
    
        public SharedDisposable(SharedDisposable<T> share) {
            lock(share.target) {
                // Check that Dispose() did not get a lock and dispose the
                //   object before we are able to obtain a reference to it.
                if(target.count == 0)
                    throw new ObjectDisposedException(nameof(share), "Target object has been disposed before being shared.");
                target = share.target;
                target.count++;
            }
        }
    
        // To be used as a temporary lock, such as in `using()`.
        // Can also be used as a synonym for copy c'tor.
        public SharedDisposable<T> Share() {
            return new SharedDisposable<T>(this);
        }
    
        public void Dispose() {
            lock(target) {
                if(--target.count == 0) {
                    //We don't care if Dispose takes a while as we're the only lock
                    //  holder of `target` so the lock isn't blocking any other threads
                    //  Unless there's some time related overhead of `lock()` ?
                    Value.Dispose();
                }
            }
        }
    
        private Target target = null;
        public T Value { get => target.value; }
    }
    

Example usage of using()

// Prevent the object from being disposed while being worked on
//   by creating a new share in a using() statement.

using(var lockShare = mySharedObj.Share()){
    //do stuff on lockShare
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This looks much better ;-) \$\endgroup\$ – t3chb0t Jul 17 '18 at 9:57
  • \$\begingroup\$ I guess you meant target.value.Dispose() instead of Value.Dispose() inside Dispose() \$\endgroup\$ – hoffmale Jul 18 '18 at 11:03
  • \$\begingroup\$ It's the same thing, there's a public Value property which is just a getter for target.Value. \$\endgroup\$ – TheBeardedQuack Jul 18 '18 at 11:07

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.