How to avoid this race condition in Reactive Extensions

Question

I have code like this in a method:

ISubject<Message> messages = new ReplaySubject<Message>(messageTimeout);

public void HandleNext(string clientId, Action<object> callback)
{
    messages.Where(message => !message.IsHandledBy(clientId))
            .Take(1)
            .Subscribe(message =>
                       {
                           callback(message.Message);
                           message.MarkAsHandledBy(clientId);
                       });
}

What is the rx'y way to code it, so that no race between MarkAsHandledBy() and IsHandledBy() may happen on multiple concurrent calls to HandleNext()?

EDIT:

This is for long polling. HandleNext() is called for each web request. The request can only handle one message and then returns to the client. Next request takes the next message and so forth.

The full code (still a work in progress of course) is this:

public class Queue
{
    readonly ISubject<MessageWrapper> messages;

    public Queue() : this(TimeSpan.FromSeconds(30)) {}

    public Queue(TimeSpan messageTimeout)
    {
        messages = new ReplaySubject<MessageWrapper>(messageTimeout);
    }

    public void Send(string channel, object message)
    {
        messages.OnNext(new MessageWrapper(new List<string> {channel}, message));
    }

    public void ReceiveNext(string clientId, string channel, Action<object> callback)
    {
        messages
            .Where(message => message.Channels.Contains(channel) && !message.IsReceivedBy(clientId))
            .Take(1)
            .Subscribe(message =>
                       {
                           callback(message.Message);
                           message.MarkAsReceivedFor(clientId);
                       });
    }

    class MessageWrapper
    {
        readonly List<string> receivers;

        public MessageWrapper(List<string> channels, object message)
        {
            receivers = new List<string>();
            Channels = channels;
            Message = message;
        }

        public List<string> Channels { get; private set; }
        public object Message { get; private set; }

        public void MarkAsReceivedFor(string clientId)
        {
            receivers.Add(clientId);
        }

        public bool IsReceivedBy(string clientId)
        {
            return receivers.Contains(clientId);
        }
    }
}

EDIT 2:

Right now my code looks like this:

public void ReceiveNext(string clientId, string channel, Action<object> callback)
{
    var subscription = Disposable.Empty;
    subscription = messages
        .Where(message => message.Channels.Contains(channel))
        .Subscribe(message =>
                   {
                       if (message.TryDispatchTo(clientId, callback))
                           subscription.Dispose();
                   });
}

class MessageWrapper
{
    readonly object message;
    readonly List<string> receivers;

    public MessageWrapper(List<string> channels, object message)
    {
        this.message = message;
        receivers = new List<string>();
        Channels = channels;
    }

    public List<string> Channels { get; private set; }

    public bool TryDispatchTo(string clientId, Action<object> handler)
    {
        lock (receivers)
        {
            if (IsReceivedBy(clientId)) return false;
            handler(message);
            MarkAsReceivedFor(clientId);
            return true;
        }
    }

    void MarkAsReceivedFor(string clientId)
    {
        receivers.Add(clientId);
    }

    bool IsReceivedBy(string clientId)
    {
        return receivers.Contains(clientId);
    }
}

Answer 1

It seems to me that you're making an Rx nightmare for yourself. Rx should provide a very easy way to wire up subscribers to your messages.

I like the fact that you've got a self contained class holding your ReplaySubject - that stops somewhere else in your code being malicious and calling OnCompleted prematurely.

However, the ReceiveNext method doesn't provide any way for you to remove subscribers. It is a memory leak at least. You tracking of client ids in the MessageWrapper is also a potential memory leak.

I'd suggest you try to work with this kind of function rather thanReceiveNext:

public IDisposable RegisterChannel(string channel, Action<object> callback)
{
    return messages
        .Where(message => message.Channels.Contains(channel))
        .Subscribe(message => callback(message.Message));
}

It's very Rx-ish. It's a nice pure query and you can unsubscribe easily.

Since the Action<object> callback is no doubt directly related to the clientId I'd think about putting the logic to prevent duplicate message processing in there.

Right now you code is very procedural and not suited to Rx. It seems like you haven't quite got your head around how to best work with Rx. It's a good start, but you need to think more functionally (as in functional programming).

---

If you must use your code as-is, I'd suggest some changes.

In Queue do this:

public IDisposable ReceiveNext(
    string clientId, string channel, Action<object> callback)
{
    return
        messages
            .Where(message => message.Channels.Contains(channel))
            .Take(1)
            .Subscribe(message =>
                message.TryReceive(clientId, callback));
}

And in MessageWrapper get rid of MarkAsReceivedFor & IsReceivedBy and do this instead:

    public bool TryReceive(string clientId, Action<object> callback)
    {
        lock (receivers)
        {
            if (!receivers.Contains(clientId))
            {
                callback(this.Message);
                receivers.Add(clientId);
                return true;
            }
            else
                return false;
        }
    }

I really don't see why you have the .Take(1) though, but these changes may reduce the race condition depending on its cause.

Answer 2

I'm not sure employing Rx like this is a good practice. Rx defines the concept of streams which requires there be no concurrent notifications.

That said, to answer your question, to avoid a Race condition, put a lock inside the IsReceivedBy and MarkAsReceivedFor methods.

As for a better approach, you could abandon the whole handling business, use a ConcurrentQueue and TryDequeue a message on receiving a request (you're only doing Take(1) - which fits a queue model). Rx can help you to give each message a TTL and remove it from the queue, but you could also do that on an incoming request.