How to handle events from other threads

Unity does not like it when I multithread, in this case using async for TCP streams. However, I didn't think I was calling an event on a different thread.

**TCP Listener**

Code (CSharp):

1.   public delegate void ChatEvent (object sender, ChatEventArgs e);
2.     public delegate void ServerEvent(object sender, ServerEventArgs e);
3.  
4.     public class TCPClientListener {
5.         public event ChatEvent OnChatMessage;
6.         public event ServerEvent OnConnect;
7.         private TcpClient tcpClient = new TcpClient ();
8.         private NetworkStream stream;
9.  
10.         private void ConnectAsync(){
11.            tcpClient.BeginConnect(host, port, ConnectCallbackAsync, tcpClient);
12.         }
13.      
14.         private void ConnectCallbackAsync(IAsyncResult ar){
15.             if(OnConnect != null)
16.                OnConnect(this, new ServerEventArgs("You connected!"));
17.              stream = tcpClient.GetStream();
18.              ReadAsync();
19.         }
20.  
21.         private void ReadAsync(){
22.           stream.BeginRead(buffer, 0, bufferSize, ReadCallbackAsync, tcpClient);
23.         }
24.  
25.         private void ReadCallbackAsync(IAsyncResult ar){
26.            string message = // convert stream byte[] to string
27.            if(OnChatMessage != null)
28.                OnChatMessage(this, new ChatEventArgs(message));
29.             stream.EndRead();
30.             ReadAsync();
31.         }
32.     }

**Unity Chat**

Code (CSharp):

1.     using UnityEngine;
2.     public class Chat : Monodevelop{
3.         public Text chatArea; // Initialized via Unity Editor
4.         private TCPClientListener client = new TCPClientListener();
5.  
6.         void Awake(){
7.             client.OnConnect += new ServerEvent(OnConnect);
8.             client.OnChatmessage += new ChatEvent(ChatMessage);
9.             ClientStart();
10.         }
11.  
12.         private ClientStart(){
13.             client.ConnectAsync();
14.         }
15.      
16.         private AppendToChat(string msg){
17.             Debug.Log(msg);  // No problem
18.             chatArea.text += "\n" + msg;  // get_enabled being called from another thread error
19.         }
20.      
21.         private void OnConnect(object sender, ServerEventArgs e){
22.             AppendToChat(e.eventInfo); // No issues
23.         }
24.      
25.         private void ChatMessage(object sender, ChatEventArgs e ){
26.             AppendToChat(e.eventInfo); // Culprit of unity error
27.         }
28.     }

I just wrote this as an example so hopefully I didn't create some error. But if you can follow, my question is this: When I eventually end up calling AppendToChat, exactly which thread am I calling it on?

When OnConnect fires, I append to the chat area with no problems.

When ChatMessage fires, Unity tells me I am calling the chatArea from another thread (via warning: "get_enabled can only be called from the main thread"). So I am not sure what thread I am on with these event listeners.

How then can I avoid calling AppendToChat on another thread ( I'm not even sure what thread it's on ).

 

I have no idea, but you may be better off posting in the Networking sub-forum where questions like this are more common.

 

I had a similar problem with google play realtime multiplayer (afaik). I did it this way:

1. when I receive a message I save the byte[] array into a queue (lock the queue when adding an element!)
   
2. In the update (or in a coroutine loop) I iterate all queued messages and dequeue and evaluate them (lock the queue when iterating over it!)

this should be enough.
Keep in mind to always lock the queue when doing any operation with it.

 

If you are using anything in an asynchronous mode, multi-threaded, then you can't access any unity objects as they require you to be on the main thread.

In your case it appears sometimes OnConnect sometimes comes back on the main thread. This might happen if the thing you're connecting to connect immediately (like say you connect to the localhost it might come back super fast, and not end up on its own thread). Subsequent calls though will definitely be on their own thread.

Really though... if you're accessing something asynchronously, you just assume all calls are on a separate thread.

Of course you could store a reference to the main thread, and compare the current thread on callback to see if you're on a different thread or not. But like I said, I usually just assume I'm on a different thread in an async callback. It's faster.

As for how to get back to the main thread.

Well you could have a Queue of Action delegates, or the values needed, or whatever is most efficient. And when you receive these messages in the ChatMessage and OnConnect events, you queue up a call to AppendToChat. Then in the 'Update' message, you check if queue has any entries, call them if they do, and then clear the queue. Don't forget to use locks so that if you receive a message mid 'Update' you don't cross access the Queue.

Something like this:

Code (csharp):

1.  
2. using UnityEngine;
3. using System.Collections.Generic;
4.  
5. public class Chat : Monodevelop{
6.  
7.     public Text chatArea; // Initialized via Unity Editor
8.     private TCPClientListener client = new TCPClientListener();
9.    
10.     private Queue<string> _appendQueue = new Queue<string>();
11.     private object _asyncLock = new object();
12.  
13.     void Awake(){
14.         client.OnConnect += new ServerEvent(OnConnect);
15.         client.OnChatmessage += new ChatEvent(ChatMessage);
16.         ClientStart();
17.     }
18.  
19.     private ClientStart(){
20.         client.ConnectAsync();
21.     }
22.  
23.     private AppendToChat(string msg){
24.         Debug.Log(msg);  // No problem
25.         chatArea.text += "\n" + msg;  // get_enabled being called from another thread error
26.     }
27.  
28.     private void OnConnect(object sender, ServerEventArgs e){
29.         lock(_asyncLock)
30.         {
31.             _appendQueue.Add(e.eventInfo);
32.         }
33.     }
34.  
35.     private void ChatMessage(object sender, ChatEventArgs e ){
36.         lock(_asyncLock)
37.         {
38.             _appendQueue.Add(e.eventInfo);
39.         }
40.     }
41.    
42.    
43.     private void Update()
44.     {
45.         if(_appendQueue.Count == 0) return;
46.        
47.         lock(_asyncLock)
48.         {
49.             foreach(var msg in _appendQueue)
50.             {
51.                 this.AppendToChat(msg);
52.             }
53.             _appendQueue.Clear();
54.         }
55.     }
56.    
57. }
58.  

In it I just queue the messages that need to be appended. Less garbage than queueing up delegates.

 

Thanks I will give this a go. Sidenote: What's the reason to use underscores for your properties? MS recommends not to
https://msdn.microsoft.com/en-us/library/ms229045(v=vs.110).aspx

 

nice example, lordofduct.
Is there a reason why you have a separate lock object instead of the queue itself?

I have one problem with your example (because I always try to make things as reusable as possible):
instead of the text part of the messages I would separate the logical parts a little bit cleaner and queue the byte[] arrays themselves (converting to chat-message callback has to be done by another class). This would allow to use the same logic for any kind of network-messages.

 

Also after reading about locks, I still don't understand how a lock prevents the threading issue.

 

lock(myObject) { } tells all the other threads that they have to wait if they want to access the locked object. The threads will sleep (if they want to use the object) and wake up again as soon as the end of the lock-block is reached.

 

Oh, I agree that I'd go about it a different way.

I was merely modifying the existing code very simply to show how it could be done. There's nothing inherently wrong with this example, just that it's rather rigid, and doesn't give a lot of room to grow.

But eh.... that's what happens when you quickly write an example in the web browser.

 

I've found the Unity community has their own coding style which doesn't always match up with Microsoft's recommendations. You'll get used to it.

In this case there is no reason to not use the appendQueue. However, it is possible that a future refactoring could expose the appendQueue publicly. At this point, it is dangerous to lock on the queue because another object could also lock the queue and lead to dead lock. Using a private object that is specifically for locking and named as such is a big clue to future developers to be careful.

It doesn't help with your specific problem, it is to prevent a new problem introduced by sharing data across threads. lordofduct solved your problem by only calling Unity framework commands inside the main thread. The way he ensured they would only be called from the mean thread is by moving your code from the callback method, into the update method. The Update method is not called from your async code, so the only way control can be here is when the Unity process calls the method each frame. If you added a call to Update from your callback method (don't do that it would be really silly, it's just to illustrate...) then you would be back at square one: Unity framework commands might be called from the child thread.

When not dealing with multithreads, you can think of memory as being static -- it will always be the way you left it. Once you introduce threads, you need to think of memory as spinning. Pretend that it could change at any moment unless you do something to stabilize it. This is what locking is for. One thread is going to change the queue but in order to do that safely, it needs to stabilize it via a lock. This prevents other threads from trying to change or view the data while it's being set. The same is true when reading the queue: you need to stabilize it so that someone doesn't come and move things around while you are looking at it.

 

Then to me it sounds like I don't need locks. I just need my event listeners in Chat to append messages to a Queue, and for Update to check the queue.

 

One of the reasons MS recommends not doing it is because it's the internal naming convention used by any of their auto-code generation.

For instance if you say:

Code (csharp):

1.  
2. public string SomeProperty { get; set; }
3.  

The compiler will auto-generate a field of type string named "_SomeProperty", and the getter and setter functions will get or set that "_SomeProperty" field.

Personally I didn't start in .Net (nor w/ Unity), in the communities and workplaces I worked, we always used "_" to represent a private field. I generally find it more readable. If I'm reading a chunk of my code and I see an underscore I know that's a field, that is local to this object, but is not local to this function.

Aside from that, if you read arguments from the .Net creators for why they made the "no underscore" rule (which comes from C/C++ communities), their general response is "we hate them... yeah, we know that they crop up in our older source (primarily because the developers of C# came from C/C++ backgrounds), but we're trying to stop that... because we hate them". They make no real reason against them, only that you COULD use the this keyword (though I use 'this', I still like the _ informing me it's a local private field, rather than say a property of a base class). And sorry... MS devs telling me to change a style I've written in for years because they "don't like it"... well I say:

 

Not quite. When I say

I mean literally at any moment, even while you are working with it. If you don't use the locks, your child thread might mess with the queue while the main thread is trying to take items out of it. It is pretty difficult to anticipate how this will behave.

 

Yep, eisenpony smacked it right on the head.

Of course I didn't have to for example code. But it's like the turn signal on my car. I just do it, I don't even think about it.

 

I'm not sure if this is correct. wouldn't a lock prevent another thread to lock that (public) object from beeing locked until the current lock block reached the end?

Your problem is that messages are received from another thread than the main thread. In that thread (whatever that thread is) you want to enqueue the incoming message.
However in the main thread (which is probably not the receiver thread) you want to do things with the queued messages.
So you need the lock.

imagine the following:
as of lordofducts code

Code (CSharp):

1.    private void Update()
2.     {
3.         if(_appendQueue.Count == 0) return;
4.      
5.         // lock(_asyncLock)
6.         {
7.             foreach(var msg in _appendQueue)
8.             {
9.                 this.AppendToChat(msg);
10.             }
11.             _appendQueue.Clear();
12.         }
13.     }

imagine right before "_appendQueue.Clear();" is called a new message is received (in the other thread). The new message would be enqueued to _appendQueue and would be cleared with all the other enqueued messages.
But if you lock it, the other thread would wait before enqueuing (hope i spelled that freaky word correctly ).

 

Not 100% sure what you mean but let me give you three reasons why you might not want to lock a publicly available object.

First, and perhaps least likely to occur here, consider the following two classes

Code (csharp):

1. class Foo
2. {
3.   public var fooQueue = new Queue<int>();
4.   public Chat chatThing = GetComponent<Chat>();
5.   public DoSomething()
6.   {
7.     lock (fooQueue)
8.     {
9.       lock (chatThing._appendQueue)
10.       { //... }
11.     }
12.   }
13. }
14.  
15. class Bar
16. {
17.   public Foo fooThing = GetComponent<Foo>();
18.   public Chat chatThing = GetComponent<Chat>();
19.   public DoSomethingElse()
20.   {
21.     lock (chatThing._appendQueue)
22.     {
23.       lock (fooThing.fooQueue)
24.       { //... }
25.     }
26.   }
27. }

This is a classic deadlock waiting to happen. If a thread enters Foo.DoSomething and gets as far as locking the fooQueue before a context switch to another thread entering Bar.DoSomethingElse we are in big trouble. The second thread will lock _appendQueue and then go to sleep when it hits the lock for fooQueue. Once thread one gets context back, it will go to sleep when it hits the lock for _appendQueue. Now we have two threads which will sleep forever. Of course, you don't need two classes with public members to reproduce this. You can dead lock in a single class using private objects. However, when you have code in the same place, it is much easier to detect these kinds of issues and see that you should lock your objects in a consistent order. When the code is spread amongst several other classes, this kind of deadlocking can become much easier because the author of Bar may not know that Foo is locking _appendQueue and even if it does, it may not be possible to know in which order it locks them.

Second, related to the first. Let's say you wrote the Chat class as part of a million LOC project. Things are working pretty well and your code has held up well against stress testing.

Suddenly, your code starts performing terribly; messages are not being printed to the log in a timely manner. After hours of debugging and staring at dependency graphs you finally stumble across this new code. written by a less experienced developer.

Code (csharp):

1. class Chatterbox
2. {
3.   public Chat chatThing = GetComponent<Chat>();
4.   public void SaySomething(string message)
5.   {
6.     lock (chatThing._appendQueue)
7.     {
8.       // count to infinity
9.       for (int i = 0; i < int.Max; i ++) ;
10.       chatThing._appendQueue.Add(message);
11.     }
12.   }
13. }

Now hopefully you won't ever have to work with anyone who likes to count pointlessly, but the slow code doesn't need to be so obvious. Maybe they are connecting to a database which is slow to spin up, or sending a web request which takes a few seconds to return. The point is, you no longer have control over the performance of your own code. Worse, you won't necessarily be able to find the offending code quickly, especially if the novice developer is only allowed to work in non-performance-critical areas, so their slow code is not detected.

Finally, and hopefully you never experience this one, say you are working on a large project which provides a plug-in framework. To protect your clients and company's good name, you always sandbox the plugins in a low trust app domain so that the plugins can't access certain resources. A colleague of yours is let go because of some.. lets call it inappropriate counting. Now this colleague is a little hurt so he goes out and writes a malicious plugin. Since the plugins are sandboxed, he can't do any serious damage. However, if he knows the source code in the main thread locks on a certain public object, he can write a plugin which locks indefinitely on the same object, causing the main thread to go to sleep forever. A lousy, though predictable, DOS attack

The main issue in all of the examples is that the object your code is locking on is public, so your code is now vulnerable to a sleep spell you don't know anything about. It's not typically an issue for small projects and there are reasons to go this route. For instance, it is quite a bit simpler than a mutex for supporting thread cooperation.

Ultimately, only locking on private objects is a rule-of-thumb to help keep threading bugs to a minimum. If you have a good reason to break this, then go for it. Just beware someone will probably pluck you on it.

 

This is completely correct. Unfortunately things are even worse than this suggests. C# code gets reduced to IL code and IL code gets reduced to native machine language. What appears to be a single line of C# might be 10 lines of IL and 100 commands in native. True context switches occur at the native level. I suspect the CLR does some protection to keep context switches at the IL level but that is just hearsay and speculation on my part. Anyways, IL is what you have to worry about, and it often looks nothing like the C# you write.

We don't even have to get that far to run into problems. Just having two threads trying to add items to the same list can cause crazy things to happen. Have a look at the Implementation of List<T>.Add()

Code (csharp):

1.         public void Add(T item) {
2.             if (_size == _items.Length) EnsureCapacity(_size + 1);
3.             _items[_size++] = item;
4.             _version++;
5.         }

You can see that _size is being increased and then _items at _size position is getting the added element. Well what happens if two threads are using the same list and a context switch happens right after thread #1 finishes _size++ ?

Now the second thread is going to do the same: increase _size (leaving a hole in the array) and insert the new item. When thread 1 wakes up again, it's going to remember having increased the _size, so it will just stuff its value into the array, overwriting thread 2's value!

Things get even worse if EnsureCapacity fails..

Code (csharp):

1.         private void EnsureCapacity(int min) {
2.             if (_items.Length < min) {
3.                 int newCapacity = _items.Length == 0? _defaultCapacity : _items.Length * 2;
4.                 // Allow the list to grow to maximum possible capacity (~2G elements) before encountering overflow.
5.                 // Note that this check works even when _items.Length overflowed thanks to the (uint) cast
6.                 if ((uint)newCapacity > Array.MaxArrayLength) newCapacity = Array.MaxArrayLength;
7.                 if (newCapacity < min) newCapacity = min;
8.                 Capacity = newCapacity;
9.             }
10.         }
11.  
12. // ...
13.         public int Capacity {
14.             get {
15.                 Contract.Ensures(Contract.Result<int>() >= 0);
16.                 return _items.Length;
17.             }
18.             set {
19.                 if (value < _size) {
20.                     ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.value, ExceptionResource.ArgumentOutOfRange_SmallCapacity);
21.                 }
22.                 Contract.EndContractBlock();
23.  
24.                 if (value != _items.Length) {
25.                     if (value > 0) {
26.                         T[] newItems = new T[value];
27.                         if (_size > 0) {
28.                             Array.Copy(_items, 0, newItems, 0, _size);
29.                         }
30.                         _items = newItems;
31.                     }
32.                     else {
33.                         _items = _emptyArray;
34.                     }
35.                 }
36.             }
37.         }

Let's say thread 1 entered the Capacity setter block at the time of context switch. Thread 2 takes over and copies everything into a new array before thread 1 wakes up. Depending on when the context switch happened, thread 1 might increase the capacity again or it might just blow up trying to copy an array that doesn't exist, or maybe it will make a second copy and then which one is used? Who knows?!

This is why it's best to consider thread shared data as "spinning". You just don't know what state it's in unless you do something to freeze it.

 

That is just amazing. It solved my problem with broadcasting messaging from a async handler, and receiving main thread erros.