Unity SDK: Delay dispose of CancellationTokenSource when quitting

[kaboing]

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking

/kind bug

/kind cleanup
/kind documentation
/kind feature
/kind hotfix
/kind release

What this PR does / Why we need it:
Quitting in Unity fires a number of events and callbacks.

MonoBehaviour.OnApplicationQuit is called before Application.wantsToQuit which leads to undesired behaviour. Application.wantsToQuit can be used to interrupt a quit process, so you can shut down gracefully. If this is utilized, the AgonesSDK has already disposed the cancellation token and future calls will fail.

We use this when controlling disruption https://agones.dev/site/docs/advanced/controlling-disruption/

Reference: https://docs.unity3d.com/6000.0/Documentation/ScriptReference/Application-wantsToQuit.html

Same solution as in UniRX: neuecc/UniRx#463

Which issue(s) this PR fixes:

Closes #

Special notes for your reviewer:

[0xaravindh]

/gcbrun

[agones-bot]

Build Succeeded 🥳

Build Id: 80def32a-2120-4a72-9f4d-2e45d0648ef0

The following development artifacts have been built, and will exist for the next 30 days:

• image: us-docker.pkg.dev/agones-images/ci/agones-controller:1.46.0-dev-9d92406
• image: us-docker.pkg.dev/agones-images/ci/agones-extensions:1.46.0-dev-9d92406
• image: us-docker.pkg.dev/agones-images/ci/agones-sdk:1.46.0-dev-9d92406-linux
• image: us-docker.pkg.dev/agones-images/ci/agones-ping:1.46.0-dev-9d92406
• image: us-docker.pkg.dev/agones-images/ci/agones-allocator:1.46.0-dev-9d92406
• Linux C++ SDK (build): agonessdk-1.46.0-dev-9d92406-linux-arch_64.tar.gz
• SDK Server: agonessdk-server-1.46.0-dev-9d92406.zip

A preview of the website (the last 30 builds are retained):

• https://9d92406-dot-preview-dot-agones-images.appspot.com/

To install this version:

git fetch https://github.com/googleforgames/agones.git pull/4075/head:pr_4075 && git checkout pr_4075
helm install agones ./install/helm/agones --namespace agones-system --set agones.image.registry=us-docker.pkg.dev/agones-images/ci --set agones.image.tag=1.46.0-dev-9d92406

[markmandel]

Calling our most recent Unity experts for review please: @ZeroParticle , @aallbrig, @kingshijie, @MaxHayman, @mollstam - does this change look good to you? 👋🏻

[mollstam]

Hello all, a bit surprised by this ping (haven't used Agones since '21) but looking at the code I do have some concerns.

I think this change per se is OK, as it does indeed address undesired behaviour when application quit is aborted / handled gracefully, but I think the code would benefit from us zooming out a notch.

I would only ever create a CancellationTokenSource (CTS) if I would pass in its Token into an async method. To me its very weird to only use the CTS for checking its cancellation status and if so throwing, I don't believe its idiomatic C# and you might as well have a bool field you flip (in this case in the new OnApplicationQuitting) and when checking if flipped just throw or noop. Using a CTS provides no benefit here.

This leads me to another very important point: CancellationTokenSource does not cancel when disposed. Here's from the .NET docs for CancellationTokenSource.Dispose:

Note that calling Dispose does not communicate a request for cancellation to consumers of the associated Token. You can communicate a request for cancellation by calling methods such as Cancel or CancelAfter.

So AgonesSdk.cancellationTokenSource is never cancelled (it's only used in this file no?) which makes that check in SendRequestAsync even more mysterious. I think the only effect it has, and maybe the original error that @kaboing was seeing ("causing future calls to fail") is trying to use a disposed CTS, which will make the .Token access throw an System.ObjectDisposedException.

If that is true I don't think the changes proposed in this PR are necessarily the fix called for. If it is as that comment in SendRequestAsync suggests: "To prevent that an async method leaks after destroying this gameObject" I think we're better of simply checking if (this == null) (which internally uses Unity's lifetime check logic) instead and getting rid of the CTS altogether.

Sorry for a long reponse, let me know what your thoughts are. Gotta run.

[ZeroParticle]

@mollstam's comments seem reasonable to me. I do have some concern that the async request handler setup we have in that file does not have any cancellation implementation. It seems reasonable to attempt to cancel any inflight requests when the game object is destroyed.

[kaboing]

@mollstam Makes sense, good observation.
You were proposing something like this for SendRequestAsync?

        public async Task<AsyncResult> SendRequestAsync(string api, string json,
            string method = UnityWebRequest.kHttpVerbPOST)
        {
            // To prevent that an async method leaks after destroying this gameObject.
            if (this == null)
            {
                return new AsyncResult { ok = false };
            }

[kaboing]

The HealthCheck shows signs of the ObjectDisposedException

https://github.com/googleforgames/agones/blob/main/sdks/unity/AgonesSdk.cs#L269-L284