0

I'm a relatively new to Unity3d C# but not to programming (Java). I'm trying to work with Threads and so far it has been quite successful as it is a lot like Java. But I'm having code lock and I'm trying to figure out why. DownloadStatus is just an enum.

The following code is called from the ThreadPoolCallback:

public static Dictionary<int, int> downloading = new Dictionary<int, int>(); ... //Look on the download list while (true) { foreach (int id in downloading.Keys) { lock(downloading){ Debug.Log("Thread " + threadIndex + " checking id: "+id); if(downloading [id]==(int)DownloadStatus.WAITING){ //This file is waiting downloading [id] = (int)DownloadStatus.DOWLOADING; Debug.Log ("Thread " + threadIndex + " is downloading " + id); //TODO: Actual downloading //Mark as done downloading [id] = (int)DownloadStatus.DONE; Debug.Log ("Thread " + threadIndex + " is done downloading " + id); }else if(downloading [id]==(int)DownloadStatus.DOWLOADING){ Debug.Log ("Thread " + threadIndex + " ignoring "+id+" since is already downloading!"); }else if(downloading [id]==(int)DownloadStatus.DONE){ Debug.Log ("Thread " + threadIndex + " ignoring "+id+" since is already downloaded!"); } } } //If you made it here, there's nothing to process Debug.Log ("Thread " + threadIndex + " closed!"); _doneEvent.Set(); break; }

If I comment the lines where I change the value of the dictionary (i.e. downloading [id]=(int)DownloadStatus.DOWLOADING;) I can see the threads going thru all the values in the dictionary. When I don't the thread stops there. The only purpose of this state is to prevent other threads from attempting the same download.

Maybe is just the wrong approach for accomplishing this.

Any idea?

Update

Code to populate downloading:

Retriever.downloading.Add(id,(int)DownloadStatus.WAITING);

Where Retriever is the name of the Thread class.

Following the suggestions by KeithS the code changed to:

public static Dictionary<int, int> downloading = new Dictionary<int, int>(); private readonly object syncObj = new object(); ... //Look on the download list foreach (int id in downloading.Keys) { lock(syncObj){ Debug.Log("Thread " + threadIndex + " checking id: "+id); if(downloading [id]==(int)DownloadStatus.WAITING){ //This file is waiting downloading [id] = (int)DownloadStatus.DOWLOADING; Debug.Log ("Thread " + threadIndex + " is downloading " + id); //TODO: Actual downloading //Mark as done downloading [id] = (int)DownloadStatus.DONE; Debug.Log ("Thread " + threadIndex + " is done downloading " + id); }else if(downloading [id]==(int)DownloadStatus.DOWLOADING){ Debug.Log ("Thread " + threadIndex + " ignoring "+id+" since is already downloading!"); }else if(downloading [id]==(int)DownloadStatus.DONE){ Debug.Log ("Thread " + threadIndex + " ignoring "+id+" since is already downloaded!"); } } } //If you made it here, there's nothing to process Debug.Log ("Thread " + threadIndex + " closed!"); _doneEvent.Set(); break; }

And here's the output (Have only 2 threads for testing purposes):

914 files to update! Thread 1 started... Thread 2 started... Thread 1 checking id: 2 Thread 1 is downloading 2 Thread 1 is done downloading 2 Thread 1 closed!

And basically stays there for ever. Same kind of behavior I had before.

Improve this question
2
  • Why don't you use coroutines for asynchronous downloads? It's the way recommended by Unity while threads are considered as dangerous. Commented Oct 24, 2014 at 11:26
  • Can you link me to some documentation on that? I'm using asynchronous downloads just within threads. But for now I moved away from the threads. Commented Oct 27, 2014 at 15:00

2 Answers 2

Reset to default
2

The most common way in Unity is to use coroutines. Although they are not threads you will get an almost thread like asynchronous behaviour (s. for example Threading in Unity for more on this).

There was a very interesting article Unity3D coroutines in detail about this at AltDevBlog but it seems to be removed. Glad to have the Internet Archive Wayback Machine:
https://web.archive.org/web/20140719082851/http://www.altdev.co/2011/07/07/unity3d-coroutines-in-detail/

Downloads can be done conveniently with the WWW class. To put this together:

public void Download (string string url) { StartCoroutine (Download_Coroutine (url)); } IEnumerator Download_Coroutine (string url) { using (WWW www = WWW.LoadFromCacheOrDownload (bundleUrl, CurrentVersion)) { while (!www.isDone) { yield return null; // optionally report www.progress to callback } if (string.IsNullOrEmpty (www.error)) { // s. doc for more options to get the content AssetBundle bundle = www.assetBundle; if (bundle != null) { // do something } } } }
Improve this answer
Sign up to request clarification or add additional context in comments.

1 Comment

Unfortunately, coroutines have massive drawbacks. Some stem from the fact that they're a bastardisation of iterators, others from the fact that you're then tied to executing on Unity's main thread.
1

First off, avoid using lock on an object that has another use in code. Best practice is to lock an object instance specifically created for the purpose:

private readonly object syncObj = new object(); ... lock(syncObj) { ...

Second, the while(true) statement is redundant as currently implemented. Each worker will go through the entire collection of downloads, until it finds a "waiting" download, it will handle that download, and then move on to the next record. Once each worker gets to the end of the collection, it will execute the code at the bottom of the loop, which will break out of it without even looping once.

Now, based just on this code (and the unseen code to populate downloading), you might have the wrong abstraction for this collection. The purpose of the multithreaded code is to divide and conquer a bunch of downloads in parallel. So, why not load these up as a Queue<Tuple<int,int>> (or even better, a ConcurrentQueue<Tuple<int,int>>), and have your worker threads pull items from the queue until there aren't any more? This way, it's the collection, not the workers or the work items, making sure that only one worker ever has a particular work item.

The worker will take the item from the queue (access to the "TryTake()" method used for this purpose is synchronized, so only one worker can be de-queuing at a time), download the file it represents, then when done, it can place the item in a ConcurrentDictionary indicating it's complete, and go back to the Queue for more until there are no more. If there's an error in downloading, the worker should catch it and either place the item back in the Queue so another worker can try, or place it in an Error collection (maybe both, based on a number of retries of the download or the nature of the error).

Improve this answer

3 Comments

Careful about using instances of the lock if you're going cross-threads. I usually see people use a readonly static object to make sure it's set once and not re-set which could result in concurrent access to code that should be locked. Another approach is to cast the dictionary as an ICollection and use the SyncRoot which is designed for this. lock ((ICollection)downloading).SyncRoot) {...}
I was unable to find a namespace for ConcurrentQueue. Seems to be .NET exclusive. I'm using Unity3D. Added that detail to the body of the question, was already a tag, sorry.
Besides the Queue suggestion above, I did the rest but it still locks. Will add the output in the question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.