Calling StartClient() from within an OnServerStopped() callback leads to inconsistent state

[xaroth8088]

Description

Calling StartClient() or StartHost() from within an OnServerStopped()/OnClientStopped() callback leads to inconsistent state.

Reproduce Steps

1. NetworkManager.Singleton.OnServerStopped += MyOnServerStoppedHandler
2. NetworkManager.Singleton.StartHost()
3. Wait for a few seconds
4. NetworkManager.Singleton.Shutdown(true)
5. Inside MyOnServerStoppedHandler, call NetworkManager.Singleton.StartClient()

Actual Outcome

A few tidbits of client state are cleared out at the tail end of NetworkManager::InternalShutdown() that were just set up inside of StartClient()/StartHost(). For example, StartClient() calls ConnectionManager.LocalClient.SetRole(false, true, this); as part of that callback, but then a moment later InternalShutdown() calls ConnectionManager.LocalClient.SetRole(false, false);.

Expected Outcome

Any of these three options would be fine:

1. (preferred) All shutdown work is complete and settled before OnServerStopped()/OnClientStopped() are called, permitting StartClient()/StartHost() to be called from within those callbacks.
2. A new OnNetworkManagerShutdownComplete callback happens when shutdown is well and truly complete.
3. StartClient()/StartHost() detect that shutdown isn't quite finished yet, and bail by returning false (preferably with some sort of error log explaining why).

Environment

• OS: N/A
• Unity Version: N/A
• Netcode Version: 1.6.0

Additional Context

I bumped into this as a problem while trying to accommodate a flow of "I used to be hosting, but then I got an invite from a friend and want to switch to being a client that's connected to their host". To work around the problem, I had MyOnServerStopped() call Invoke(nameof(OnServerFullyStopped), 0.01f), and then called StartClient() from within OnServerFullyStopped().

This workaround caused an issue for me in a different way, though, because SteamNetworkingSocketsTransport does a similar workaround during their shutdown. Critically, they wait for 0.1f seconds before finishing the transport's cleanup. This resulted in a race condition where my game would be halfway through connecting to its new host when the transport would wrap up shutting itself down.

For the time being, I can work around that problem by simply waiting longer for everything to settle before calling StartClient(), but clearly that's suboptimal all around.

All of that said, if I'm simply missing something in the docs on how my code can know that NetworkManager is really, truly safe to call StartClient()/StartHost(), or if I'm going about it wrong and should instead destroy/recreate my NetworkManager for this particular flow, please let me know!

[NoelStephensUnity]

Hi @xaroth8088,

So, the shutdown notifications were intended to be used as a way to receive a notification that if you needed to handle any client or server specific "shutdown clean up" (or perhaps begin to load back to a different scene or lobby...etc) could begin at that time. But we seem to have a lack of documentation on this so that is noted and we will look to improve the documentation for these notifications.

I would suggest something like the below pseudo code to handle a direct migration to a different network session:

    public class InviteHandler : MonoBehaviour
    {
        private WaitForFixedUpdate m_WaitForFixedUpdate = new WaitForFixedUpdate();

        private void Start()
        {
            NetworkManager.Singleton.OnServerStopped += OnServerStopped;
        }

        private void OnServerStopped(bool obj)
        {
            NetworkManager.Singleton.OnServerStopped -= OnServerStopped;
            // Defer starting the client until the next update loop/frame
            StartCoroutine(DelayedClientStart());
        }

        private IEnumerator DelayedClientStart()
        {
            // Wait for the next fixed update (or any specified period of time)
            yield return m_WaitForFixedUpdate;
            // Start the client
            NetworkManager.Singleton.StartClient();
            yield break;
        }
    }

Basically, you only need to defer the starting of the NetworkManager as a client until the next frame (to be the safest). This pattern works with UnityTransport, but you might need to tweak it slightly (i.e. maybe use a WaitforSeconds as opposed to WaitForFixedUpdate ) if you are using a third party transport other than UnityTransport.

This isn't a bug as much as a lack of documentation, and I am sorry if you spent more time on this than you had planned.
We will work on getting the documentation updated along with the API documentation in order to further clarify how those event notifications should be used and what not to do (i.e. start another instance during the event notification).

[xaroth8088]

Thank you! That makes sense, and your pseudocode is very similar to the workaround I put in place, so I think I'm good on that front now.

I do have to add that it'd be great to have this all baked into NetworkManager, though. I'm picturing a new "NetworkManager and its attached transport are now done with all of their own cleanup, and is ready to have .Start[Host/Server/Client]() called" event (though I do understand that it'd require some thinking about the shutdown state machine vis a vis the interface for 3rd-party transports in order to do correctly).

Would you like me to file that as a separate enhancement request?

[NoelStephensUnity]

@xaroth8088

Would you like me to file that as a separate enhancement request?

So, I have had time to sleep on this and am contemplating a more "universal" event:
NetworkManager.ShutdownComplete

That would be invoked at the very end of NetworkManager.ShutdownInternal.

This would allow one to know precisely when you could start the NetworkManager again.

So it would then have:

• OnSteverStopped: Lets you know when a server-host instance is stopped and when you can clean up any project specific server related stuff.
• OnClientStopped: Lets you know when a client instance is stopped and when you can clean up any project specific client related stuff.
• ShutdownComplete: Lets you know that NetworkManager is 100% shutdown (i.e. you are ok to start a new session).