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

http://stackoverflow.com/questions/28715625/is-it-safe-to-use-field-inside-and-outside-synchronized-block

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

问题：

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.
}

 

 

 

解答：

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 _mailQueueRepositoryavoiding mix usage.

 

Actually, it's the method which reads the queue and sends emails - not the repository itself - that I'm trying to prevent concurrent access to. I want threads to be able to write to the repository while one thread is reading emails and sending them. But I guess ReSharper can't tell that, so it warns me. I know that I'm actually fine with unsynchronized access to that resource, so I guess I'm safe to ignore the warning? – OutstandingBill

@OutstandingBill - From resharper : "Warnings In addition to compiler errors and warnings, ReSharper displays its own warnings that don't prevent your code from compiling but may nevertheless represent serious coding inefficiencies". Resharper is intelligence enough to say contradictory usage but you are the one know what actual usage is. If you are confirmed to use the resource well managed you could ignore the warning :) – KcDoD

@OutstandingBill Additionally you might be interested in re-tructuring the method to take parameters extracted from the field , rather than using it in mix context – KcDoD

 

作者:Chuck Lu    GitHub