7
\$\begingroup\$

Assumptions

Basically, a Connection class has a "Disconnect" event. Subscribing to this event isn't thread-safe, because the disconnection may fire from another thread right before I subscribe. So checking before the subscription doesn't help.

Checking for the disconnect after subscription doesn't help either because the event may have fired in the meanwhile (2 threads might execute the same "observer" twice).

(My) Solution:

An event always fired once (and only once), even if the event itself already happened before Source is on GitHub as well.

Questions:

Are there other simpler solutions addressing this? (by simpler I mean from an outside or usage perspective)

Do you see any race conditions or things that could go wrong?

Maybe you have optimizations or simplifications to add?

Input is highly appreciated!

/// <summary>
/// Triggers if the event is invoked or was invoked before subscribing to it.
/// <para> Can be accessed safely by multiple threads.</para>
/// </summary>
public class AutoInvokeEvent<Sender, Args>
{
    public delegate void EventHandle(Sender sender, Args arguments);

    /// <summary>
    /// Handle will be invoked if the event was triggered in the past.
    /// <para>Unsubscribing happens automatically after the invocation and is redundant if done from the event handle.</para>
    /// </summary>
    public event EventHandle Event
    {
        add
        {
            if (!Subscribe(value))
                value(m_sender, m_eventArgs);
        }
        remove { InternalEvent -= value; }
    }

    private event EventHandle InternalEvent;

    // this is my personal lock implementation. in this case it is used like any other lock(object) so just ignore it
    private SafeExecutor m_lock = new SingleThreadExecutor();
    private volatile bool m_invoked = false;

    Sender m_sender;
    Args m_eventArgs;

    /// <summary>
    /// Invokes all subscribed handles with the given parameters. 
    /// <para>All calls after the first are ignored.</para>
    /// </summary>
    public void Invoke(Sender sender, Args args)
    {
        GetEventHandle(sender, args)?.Invoke(m_sender, m_eventArgs);
    }

    private EventHandle GetEventHandle(Sender sender, Args args)
    {
        return m_lock.Execute(() =>
        {
            if (m_invoked)
                return null;

            m_sender = sender;
            m_eventArgs = args;
            m_invoked = true;
            EventHandle handle = InternalEvent;
            InternalEvent = null;
            return handle;
        });
    }

    /// <returns>Returns true if subscription was successful and false if handle needs to be invoked immediately.</returns>
    private bool Subscribe(EventHandle handle)
    {
        return m_lock.Execute(() =>
        {
            if (!m_invoked)
                InternalEvent += handle;

            return !m_invoked;
        });
    }
}

How the class could be used :

class Connection
{
    public AutoInvokeEvent<object, EndPoint> OnDisconnect = new AutoInvokeEvent<object, EndPoint>();

    public Disconnect()
    {
        OnDisconnect.Invoke(this, endpoint);
    }
}

void main()
{
    Connection connection = new Connection();
    connection.OnDisconnect.Event += DoStuffOnDisconnect;
}

void DoStuffOnDisconnect(object sender, EndPoint endpoint) { }
\$\endgroup\$
  • \$\begingroup\$ What do you need this actually for? I cannot come up with any use case. \$\endgroup\$ – t3chb0t Nov 12 '18 at 20:34
  • \$\begingroup\$ pretty much for what i described in the original post, a threadsafe disconnect event which is always called, even if the event fired in the past but never twice. it is a pretty convenient guarantee on which i can build other logic in a highly multithreaded environment. \$\endgroup\$ – Oachkatzl Nov 12 '18 at 21:06
  • \$\begingroup\$ Mhmm, still not getting it. I was hoping you could name the exact application you're going to use this for but this explanation is too general and I cannot place it anywhere. Reading files, calculating something, downloading something, I don't know, you must be using it for something specific? \$\endgroup\$ – t3chb0t Nov 13 '18 at 4:50
  • 1
    \$\begingroup\$ oh, yes - it is a server which lets users access and control their PC with an android device. i am using the same principle for all my connections though. The object that is using the connection is for example a User-Object which needs to be aware if the user is still connected and also react to their disconnect in general. This is where this event comes into play. \$\endgroup\$ – Oachkatzl Nov 13 '18 at 9:54
  • \$\begingroup\$ @Oachkatzl: Just posted my own followup / possible implementation \$\endgroup\$ – firda Nov 21 '18 at 19:05
2
\$\begingroup\$

Thread safety

Both Subscribe and GetEventHandle use lock and and+remove on field event are thread-safe too. You can even remove the volatile from m_invoked because you only ever access it under the lock from those two methods.

API and naming

Event and Invoke are pretty obvious, especially with the description. I would only change the signature of Invoke to return bool (true = invoked, false = not invoked, already done before) to better reflect the fact, that it won't throw exception if you try to invoke it multiple times (could as well be named TryInvoke, maybe changing Invoke to throw exceptions if invoked multiple-times).

GetEventHandle does not sound as good name to me, because it does much more than just getting the handle, it stores sender+args, takes current handle and clears it. I would either use it (and Subscribe) directly where they are used (only one place for each), which also solves naming-problem, or make them local functions.

AutoInvokeEvent hmm, how about OneShotEvent? Although it automatically calls subscribed handlers even after the event was fired, this is more about how it does it under the hood, than what the API is about - it will execute the handler exactly once when it is fired or if it already was fired.

Optimization

I don't think any optimization is necessary. You would have to first specify (and or test/profile) how often is the event subscribed compared to invoking it, to even have some direction in which to optimise. I can see you have switched to using some read-write-lock in the implementation you currently have on GitHub, makes sense (concurrent subscriptions, if that is trully important to you). Could as well be implemented by double-checked-locking, thanks to the fact this is one-shot event, but I don't think it is worth it, too easy to make a mistake, very hard to make it right. (The idea is to have volatile object m_lock and make it null when you fire the event, instead of using that bool m_invoked.)

\$\endgroup\$
  • \$\begingroup\$ thanks for the response! great idea with the bool return for the Invoke()-function. i will add that in the future. Quite funny: the class was once called OneShotEvent. It is probably fitting better especially from an "outside" perspective. \$\endgroup\$ – Oachkatzl Nov 21 '18 at 13:41
  • \$\begingroup\$ i thought about it a little more and i don't think i can return a bool there. it would destroy the atomic behavior of the one-liner and thus require more locks which is a performance hit. also the bool return value will usually not help a big deal i think. events usually don't care if nobody is listening, so i would rather not trade this information for lesser performance. \$\endgroup\$ – Oachkatzl Nov 21 '18 at 13:48
  • \$\begingroup\$ i will remove the volatile from m_invoked and will have a look at the optimization proposals later on, thanks again. :) \$\endgroup\$ – Oachkatzl Nov 21 '18 at 13:51
  • \$\begingroup\$ You can maybe return a pair of bool + delegate if you want to use the executor. It can be written without any penalty, no extra locking needed, you only need to distinguish between null becase m_invoked was true and null because nobody subscribed. \$\endgroup\$ – firda Nov 21 '18 at 13:53
  • \$\begingroup\$ that is true, this way it could be implemented without a lock. on the other hand i still think an event should not care if someone is listening. i will however keep it in mind if i encounter a situation in which i would need to know if someone called the invoke previously! :) \$\endgroup\$ – Oachkatzl Nov 21 '18 at 13:59

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.