Is it safe to use field inside and outside synchronized block?

Question

Background

Our app sends emails which are queued in a database table. We've had some instances of duplicate emails being sent, so I'm implementing a lock to prevent multiple threads from sending emails simultaneously.

ReSharper is warning me that:

the field is sometimes used inside synchronized block and sometimes used without synchronization

Question

Why is ReSharper telling me this, and why might I be worried about it?

Code

Here's my (abridged) code:

private readonly IMailQueueRepository _mailQueueRepository = new MailQueueRepository();
private static object _messageQueueLock = new object();

public void SendAllQueuedMessages(IPrincipal caller)
{
    lock (_messageQueueLock) // Prevent concurrent callers
    {
        var message = _mailQueueRepository.GetUnsentMessage();
        while (message != null)
        {
            SendQueuedMessage(message);
            message = _mailQueueRepository.GetUnsentMessage();
        }
    }
}

public void SendQueuedMessage(IMessage message)
{
    // I get the ReSharper warning here on _mailQueueRepository
    var messageAttachments = _mailQueueRepository.GetMessageAttachments(message.Id);
    // etc.
}

Answer 1

Problem scenario :

We've had some instances of duplicate emails being sent, so I'm implementing a lock to prevent multiple threads from sending emails simultaneously.

So you are using Lock() to prevent this happening, that means you need to synchronize threads accessing a common resource which in this case _mailQueueRepository

But again in the same code you use _mailQueueRepository without a Lock

 // I get the ReSharper warning here on _mailQueueRepository
    var messageAttachments = _mailQueueRepository.GetMessageAttachments(message.Id); // <== Accessed without a lock

So it's a warning to tell that your valuable resource is accessed in two different forms : one as synchronized(thread safe) and other non-synchronized(non thread safe).

And it's a warning that let you know(or let you identify) issues that could arise from this contradictory usage of the resource _mailQueueRepository. Choice is yours to either make all usages of _mailQueueRepository synchronized (use with a lock and warning will be gone) or manage not to run for race conditions.

Additionally you might consider to re-structure the codes in such a way that your SendQueuedMessage() is called with parameters which are extracted from _mailQueueRepository avoiding mix usage.

Answer 2

ReSharper can not tell (or guarantee) that SendQueuedMessage() is only called from within a synchronized block. So as far as it is concerned, other code might call SendQueuedMessage() without synchronization, and _mailQueueRepository is being used in SendQueuedMessage().

If you are sure that no other code (inside or outside the containing class) calls this method, or you've made sure all calls from within the class to SendQueuedMessage() are also synchronized using the same lock object, you're ok. If no other code outside your class actually needs this method, I would suggest you make it private.