3
\$\begingroup\$

In my loading scene I want to run multiple functions (IO from the disk) and start the app after they are completed.

This is in my loading class. (Note: This is unity 3d and the public variables to get access to other objects is based on Unity best practices).

public Logger MyLogger;
public Parser MyParser;
// ... more

private int coroutineCount = 0;

void Start()
{
    MyLogger.LogLastMonth(Callback);
    coroutineCount++;

    MyParser.ParseFiles(Callback);
    coroutineCount++;
}

public void Callback()
{
    coroutineCount--;

    if (coroutineCount <= 0)
    {
        SceneManager.LoadScene("GameScene");
    }
}

I do something similar in both Logger and Parser (counting coroutines and decrementing when they finish before invoking the callback received). Is there a better way to do this when I need to expand the number of functions I'm calling?

\$\endgroup\$
  • \$\begingroup\$ There is no such Unity Best Practices! People normally make variables public, so they show up in the inspector. You can achieve the same result by keeping them private and putting SerializeField behind them. \$\endgroup\$ – Behnam Rasooli May 18 '17 at 13:47
5
\$\begingroup\$
  1. Your code is not thread-safe on many levels. Even if you replace regular increments/decrements with methods from Interlocked class, you will still need additional synchronization to make sure, that Callback is not called while Start is running.
  2. Term coroutine has a specific meaning in context of Unity, so you should probably use different word to avoid confusion.
  3. I don't know what is the recommended way of doing these things in Unity. In absence of TPL (I assume Unity still uses .Net 3.5), the easiest solution I can think of, that does not require good understanding of threading, is to return WaitHandles from your async methods:

    void Start()
    {
        var handles = new List<WiatHandle>();
        handles.Add(MyLogger.LogLastMonth());
        handles.Add(MyParser.ParseFiles());
        //etc...
        ThreadPool.QueueUserWorkItem(_ => 
        {
            WaitHandle.WaitAll(handles.ToArray());
    
            //at this point you might want to dispose handles 
            //if you do not plan to re-use them
    
            //Make sure this call is thread-safe.
            //If it is not, you will have to dispatch this call back to main thread
            // or set some bool field here, and move the call to Update.
            SceneManager.LoadScene("GameScene");
        });
    }
    
\$\endgroup\$
  • \$\begingroup\$ Thanks for point one. Unity runs everything on a single thread unless specified (and they recommend not creating additional threads), but I see the poor design choice. The choice of coroutine is that each of those calls uses coroutines to do the tasks "async" (it's still on a single thread in unity). Thanks for example three. I didn't know about it before so I'll look into it and see if it's a better choice than coroutines. \$\endgroup\$ – i_is_it Mar 6 '17 at 9:01
  • \$\begingroup\$ Unity itself isn't thread-safe. \$\endgroup\$ – Denis Mar 6 '17 at 14:57
  • \$\begingroup\$ @Denis Unity isn't thread safe but coroutines are all run on the main thread eliminating race conditions. I do agree my implementation was poor hence asking here. I'm currently using a List<Action> to add the functions and run them and count callbacks (but it's still not thread safe). \$\endgroup\$ – i_is_it Mar 7 '17 at 8:17
  • 1
    \$\begingroup\$ I think it could be made threadsafe with interlock by incrementing at the start of Start() and decrementing at the end, but I certainly wouldn't want to claim that that's a better approach than using wait handles. \$\endgroup\$ – Peter Taylor Mar 7 '17 at 10:50
  • \$\begingroup\$ @PeterTaylor yes, I agree. Calculating totalcoroutineCountat the start of Start should do the trick if Interlocked.Decrement is used inside the callback. The only downside is that this approach is hard to maintain: every time you add/remove a "couroutine" you must remember to modify the initial count. But it is a simple fix, that does not require a lot of refactoring, so it is definitely an option that should be considered. \$\endgroup\$ – Nikita B Mar 7 '17 at 11:27
2
\$\begingroup\$

I agree with Nikita's observation about thread safety. It might be instructive to see how to do this with Interlocked, although I'm not convinced that it's necessarily a better solution than WaitHandle. It's certainly fragile - I had to correct some mistakes in my first version of this answer.

The potential threading problems are:

  1. ++ and -- read, alter a register, and write, allowing another thread to change the value in between the read and write. This is directly solved with Interlocked.
  2. If the callback is interleaved with the Start method you could get a situation where the callback count is decremented to zero even though there are still initialisation tasks to perform, causing LoadScene to be called early. This is solved by making Start itself an initialisation task.
  3. The increment must be done before registering the callback, lest the callback decrement before Start has done the increment.

Note that I prefer to use try/finally with manual synchronisation, although there's certainly an argument that if the body of Start throws an exception then we don't mind never loading the scene.

I've made Callback private because it should never be called except as a callback, and Start has access to it even as a private method.

I'm also tempted to make Callback abort with a fatal error if the number of incomplete actions is less than 0, because that indicates a major logic error which you want to catch before taking the project into production.

private int incompleteActions = 0;

void Start()
{
    Interlocked.Increment(ref incompleteActions);
    try
    {
        Interlocked.Increment(ref incompleteActions);
        MyLogger.LogLastMonth(Callback);

        Interlocked.Increment(ref incompleteActions);
        MyParser.ParseFiles(Callback);
    }
    finally
    {
        Interlocked.Decrement(ref incompleteActions);
    }
}

private void Callback()
{
    if (Interlocked.Decrement(ref incompleteActions) <= 0)
    {
        SceneManager.LoadScene("GameScene");
    }
}
\$\endgroup\$

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.