2
\$\begingroup\$

Preamble: it's a self-assigned and pure syntetic task to learn (and remember what I already knew) C# threads and synchronization and data structures.

The original question was here https://stackoverflow.com/questions/16458937/synchronization-of-remote-files-download

The short task explanation:

Let's say I have a dictionary <string, string> that represents a path (http) to a file by some key, ie:

foo => http://domain.tld/file1
bar => http://domain2.tld/file2

And I'd like to implement a class that will implement an interface with 2 methods:

String Rand();
String Get(String key);

The first method would pick the file randomly from all the available, and the Get would return a particular file, or to be precise - local path to a downloaded file.

The class should be thread-safe, so that if several threads request the same key with Get() or the Rand() picks the same item - then only one thread should actually download a file to a local drive, or the path should be retrieved immediately if a file has already been downloaded.

My proof of concept solution is:

class Downloader
{
    private readonly IDictionary<string, string> _map;
    private IDictionary<string, string> _storage = new Dictionary<string, string>();
    private ConcurrentDictionary<string, Task<string>> _progress = new ConcurrentDictionary<string,Task<string>>();

    public Downloader(IDictionary<string, string> map)
    {
        _map = map ?? new Dictionary<string, string>();
    }

    public string Get(string key)
    {
        if (_storage.ContainsKey(key))
        {
            return _storage[key];
        }

        Task<string> task;
        if (_progress.TryGetValue(key, out task))
        {
            return task.Result;
        }

        task = _retrieveFile(key);

        if (!_progress.TryAdd(key, task))
        {
            return Get(key);
        }

        task.Start();
        _storage[key] = task.Result;
        return task.Result;
    }

    private Task<string> _retrieveFile(string key)
    {
        string path;

        if (!_map.TryGetValue(key, out path))
        {
            throw new ArgumentException("The specified key wasn't found");
        }

        return new Task<string>(k =>
        {
            Console.WriteLine("Started retrieving {0}", k.ToString());
            System.Threading.Thread.Sleep(3000);
            Console.WriteLine("Finished retrieving {0}", k.ToString());
            return k.ToString() + " local path";
        }, path);
    }
}

The copy is at http://pastebin.com/LJnK7FB6

How could I make it better? Any particular problems with the code?

In the comments @I4V pointed out to some problems at https://stackoverflow.com/questions/16458937/synchronization-of-remote-files-download#comment23617420_16460673 but seems like I'm missing the point

\$\endgroup\$
1
\$\begingroup\$

If you change _retrieveFile to return a Task that's already started, you could simplify the whole Get() into just:

return _progress.GetOrAdd(key, _retrieveFile).Result;

Also, it seems that Downloader with empty map doesn't make sense, so you should check for that in your constructor, instead of using ??.

\$\endgroup\$
  • \$\begingroup\$ That's a nice point actually, but shouldn't it be _retrieveFile()? \$\endgroup\$ – zerkms May 9 '13 at 21:37
  • \$\begingroup\$ @zerkms No, there is an overload that accepts Func. Though I assumed that it's guaranteed that the delegate won't be executed more than once, which, according to the documentation is not true. So my code might not be good enough for you. \$\endgroup\$ – svick May 9 '13 at 21:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.