3
\$\begingroup\$

Giving a review to Oachkatzl and recently about some other synchronization problems (Lazy with invalidate and some node-locking list), I wanted to test my skills and redesign it in most light-weight way I can muster.

I am especially concerned about thread-safety here, because there is NO LOCK statement, only Interlocked.CompareExchange in a loop, that is used for event fields anyway. There is only one field - object _state to prevent torn-down reads (e.g. by overwriting sender+args without proper lock and knowing for sure we won to do the invoke).

Having single field also makes me consider changing this all to struct, because it will be used as private field for public event anyway.

Last thing I was thinking about was to maybe remove the runtime-typechecks (state is Invoked), but do not think it is worth it (would probably need wrapping the delegates in another class and using interface or common base class).

/// <summary>
/// Thread-safe event manager that ensures that subscribers get notified once after the event happens,
/// even if the event already happened.
/// </summary>
/// <typeparam name="Sender">Sender of the event</typeparam>
/// <typeparam name="Args">Additional arguments passed to subscribers</typeparam>
public class OneShotEvent<Sender, Args>
{
    public delegate void Handler(Sender sender, Args args);

    // helper class to store sender and args when invoked
    private class Invoked
    {
        public Sender sender;
        public Args args;
        public Invoked(Sender sender, Args args)
        {
            this.sender = sender;
            this.args = args;
        }
    }

    // Delegate (subscribers) or Invoked (sender and args)
    private object _state;

    /// <summary>
    /// Subscribers will be notified once after the event happens,
    /// can be notified immediately if the event already happened.
    /// </summary>
    public event Handler Event
    {
        add
        {
            var state = _state;
            object prev;
            do
            {
                if (state is Invoked invoked)
                {
                    value(invoked.sender, invoked.args);
                    return;
                }
                prev = state;
                state = Interlocked.CompareExchange(ref _state, Delegate.Combine((Delegate)state, value), prev);
            }
            while (prev != state);
        }
        remove
        {
            var state = _state;
            object prev;
            do
            {
                if (state is Invoked)
                    return;
                prev = state;
                state = Interlocked.CompareExchange(ref _state, Delegate.Remove((Delegate)state, value), prev);
            }
            while (prev != state);
        }
    }

    /// <summary>
    /// Notify subscribers if not already done.
    /// </summary>
    /// <param name="sender">Sender of the notification</param>
    /// <param name="args">Arguments of the notification</param>
    /// <returns>true if no invoke was called before, false if already invoked</returns>
    public bool TryInvoke(Sender sender, Args args)
    {
        var state = _state;
        if (state is Invoked)
            return false;

        var invoked = new Invoked(sender, args);
        object prev;
        do
        {
            prev = state;
            state = Interlocked.CompareExchange(ref _state, invoked, prev);
            if (state is Invoked)
                return false;
        }
        while (prev != state);

        ((Handler)state)?.Invoke(sender, args);
        return true;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Additional info can be found in this chat (me and oachkatzl talking about the fragility of these lock-less solutions, how seemingly innocent modification involving double-read can destroy the correctnes of the code). \$\endgroup\$ – firda Nov 22 '18 at 10:42
  • \$\begingroup\$ USAGE: This is primarily designed for events like connection closed or object removed/disposed. You can get such object from e.g. concurrent collection and it could already be closed/removed/disposed at the time you wish to subscribe to the event. This ensures correctnes (and saves you some checks) in these scenairos. \$\endgroup\$ – firda Nov 23 '18 at 9:31

Your Answer

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

Browse other questions tagged or ask your own question.