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

Question

Background

Our application sends emails queued in a database table. We had several copies of duplicate emails sent, so I implement a lock to prevent simultaneous sending of emails by e-mail.

ReSharper warns me that:

the field is sometimes used inside a synchronized block, and sometimes it is used without synchronization

Question

Why is ReSharper telling me this, and why can I worry about it?

the code

Here is my (shortened) 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 had several instances of duplicate emails sent, so I implement a lock to prevent multiple streams of messages from being sent simultaneously.

So, you use Lock() to prevent this, which means that you need to synchronize the threads by accessing the _mailQueueRepository , which in this case is _mailQueueRepository

But then again in the same code you are using _mailQueueRepository without Lock

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

So, this is a warning that your valuable resource is available in two different forms: one as synchronized (thread-safe) and the other non-synchronized (insecure thread).

And this is a warning that lets you know (or allow you to identify) problems that may arise as a result of this conflicting use of the _mailQueueRepository resource. The choice is yours, either for all uses of _mailQueueRepository synchronized (use with Lock , and the warning will not), or will not work in race conditions.

In addition, you might consider restructuring the codes so that the called SendQueuedMessage() called with parameters that are extracted from _mailQueueRepository , avoiding the use of the mix.

Answer 2

ReSharper cannot say (or guarantee) that SendQueuedMessage() is called only from a synchronized block. So, as far as it is known, other code can call SendQueuedMessage() without synchronization, and _mailQueueRepository used in SendQueuedMessage() .

If you are sure that no other code (inside or outside the class containing the class) calls this method, or you make sure that all calls from the class in SendQueuedMessage() also synchronized using the same lock object, fine. If no other code outside your class needs this method, I would suggest you make it private.