How to prevent other threads to change a string?

Question

I created a class that contains the configuration of an application, so that multiple threads can access the values in it. Obviously, I perform locking within the properties and the methods that set or read these values.

public class Settings
{
    private readonly object m_BackupServersLocker = new object();
    private readonly List<Uri> m_BackupServers = new List<Uri>();

    private readonly object m_ExcludedFileExtensionsLocker = new object();
    private readonly List<string> m_ExcludedFileExtensions = new List<string>();

    /// <summary>
    /// The working threads use this property to get the addresses of the remote backup servers.
    /// </summary>
    public IEnumerable<Uri> RemoteBackupServers
    {
        get
        {
            lock (m_RemoteBackupServersLocker)
            {
                List<Uri> endpoints = new List<Uri>();
                foreach (var uri in m_RemoteBackupServers)
                {
                    string uriString = string.Copy(uri.ToString());
                    endpoints.Add(new Uri(uriString));
                }
                return endpoints;
            }
        }
    }

    /// <summary>
    /// This method is invoked by the thread which reads the configuration from file.
    /// </summary>
    /// <param name="uri"></param>
    public bool InsertRemoteBackupServer(Uri uri)
    {
        lock (m_RemoteBackupServersLocker)
        {
            if (uri == null) return false;
            return m_RemoteBackupServers.Add(uri);
        }
    }

    /// <summary>
    /// This method is invoked by the thread which reads the configuration from file.
    /// </summary>
    /// <param name="uri"></param>
    /// <returns></returns>
    public bool RemoveRemoteBackupServer(Uri uri)
    {
        lock (m_RemoteBackupServersLocker)
        {
            if (uri == null) return false;
            return m_RemoteBackupServers.Remove(uri);
        }
    }

    /// <summary>
    /// The working threads use the property to get the list of excluded extensions.
    /// The property is also invoked by the thread which reads the configuration from file, in order to update the exclusion list.
    /// </summary>
    public IEnumerable<string> ExcludedFileExtensions
    {
        get
        {
            lock (m_ExcludedFileExtensionsLocker)
            {
                List<string> temp = new List<string>();
                foreach (var extension in m_ExcludedFileExtensions)
                {
                    string extString = string.Copy(extension);
                    temp.Add(extString);
                }
                return temp;
            }
        }
        set
        {
            lock (m_ExcludedFileExtensionsLocker)
            {
                m_ExcludedFileExtensions.Clear();
                foreach (var extension in value)
                {
                    temp.Add(extension);
                }
                return temp;
            }
        }
    }
}

In order to return the IEnumerable<Uri> and the IEnumerable<string>, I performed the copy of the strings using the string.Copy method. But is it really necessary to make that copy? I decided to make a copy of strings according to the following reasoning: if the properties simply return the member attribute (ie a reference to that attribute), the reader threads could change them, so I decided to return a deep copy of these lists.

However, the strings are immutable, so is it unnecessary to make a copy of these lists by copying each string and each uri from them? In other words, if I change the ExcludedFileExtensions property in the above sample as follows, then could a reader thread change the original strings within the m_ExcludedFileExtensions variable?

public IEnumerable<string> ExcludedFileExtensions
{
    get
    {
        lock (m_ExcludedFileExtensionsLocker)
        {
            return new List<string>(m_ExcludedFileExtensions);
        }
    }
}

Answer 1

Your implementation of the ExcludedFileExtensions method, is doing some things you may not have to do.

You only need to return a copy of the list contents on a get, if

1. There is a chance that the contents of the list changes by operations within your Settings class (i.e. you Add or Remove items), or
2. You want to prevent clients from casting it to a List<string> and then using it to Add or Remove elements.

Otherwise you can simply do:

public IEnumerable<string> ExcludedFileExtensions
{
    get
    {
        var current = m_ExcludedFileExtensions;
        return current;
        // If it could change/you want to prevent client casts:
        // return an immutable copy.
        // return current.ToArray();
    }
    set
    {
        // Personally I find a setter replacing the entire contents
        // rather odd, but then again I don't know your use case.
        var newList = value != null ? value.ToList() : new List<string>();
        m_ExcludedFileExtensions = newList; 
    }
}

You will also have to change the declaration of m_ExcludedFileExtensions to:

private volatile List<string> m_ExcludedFileExtensions = new List<string>();

And yes, strings are immutable, so you will never have to clone them when returning them.

Answer 2

Take a look at the ReadOnlyCollection<T> type.

Here's the MSDN Article for it: https://msdn.microsoft.com/en-us/library/ms132474.aspx

Essentially,

return new ReadOnlyCollection<string>(m_ExcludedFileExtensions);

will provide a wrapper around your list without doing a deep copy, but prevent anyone from externally modifying the underlying list. And, of course, strings themselves are immutable.