PUN2 Add/Remove/UpdateCallbackTargets implementation can lead to multiple callbacks

JaekSmith

Hello,

During my connection sequence, I call AddCallbackTarget and things work - e.g. it's added at the next internal call to UpdateCallbackTarget. During my disconnect sequence I disconnect, then call RemoveCallbackTarget ... but UpdateCallbackTarget is never called as things are basically inactive - so at this point the target is in the remove set, but not removed. Start my connection sequence which includes a call to AddCallbackTarget ... which removes the target from the remove set and adds it to the add set, noting that the target never got removed in an UpdateCallbackTarget call. Thus, at the next UpdateCallbackTarget, the target is added.
Thus, each time I disconnect ... then reconnect, I get repeated calls matching the number of times I connected.
As a quick fix I modded each of the UpdateCallbackTargets methods in LoadBalancingClient.cs and, int he add loop, gated the add call with a presence check first, ex:

        private void UpdateCallbackTargets()
        {
            if (this.targetsToAdd.Count != 0)
            {
                foreach (IConnectionCallbacks target in this.targetsToAdd)
                {
                    if (!this.Contains(target)) // CHECK ADDED HERE
                    {
                        this.Add(target);
                    }
                }
            }
            ...

Another potential implementation might be to modify the Add/RemoveCallbackTarget methods something like...

	public void AddCallbackTarget(IConnectionCallbacks target)
        {
            bool removed = this.targetsToRemove.Remove(target);

            if (!removed) // Already a target, so don't re-add
            {
                this.targetsToAdd.Add(target);
            }
        }

(Similar/opposite for RemoveCalbackTarget, of course).
Note, however, that this second form could fail if client called called RemoveCallbackTarget on a target that was never added, then called AddCallbackTarget on that target before UpdateCallbackTargets was called.

Let me know if any further info is needed.

Thanks for reviewing, etc.

Tobias

Thanks for the explanation. Yes, I see what's happening.
Tricky case.

I am tempted to suggest you only add/remove the callback only once during the lifetime of the application and not on disconnected.

The Contains() check should be fine for your case, right?

JaekSmith

(At first I did gate the add and comment out the remove ... though this couples the implementation ... to undo the coupling, I could create an adapter layer where the adapter remains added and I add/remove from the adapter .. but that's a little silly since PUN could do that).

Yeh - the Contains() check is sufficient and is the stronger implementation.

Thanks

Tobias

We think it makes sense to have this check, so we'll come up with a solution in the next update or the one after that. For the time being, just keep the .Contains().

Thanks.

JohnTube

Hi @JaekSmith,

Thank you for choosing Photon and for your report.

@Tobias submitted PUN 2.14 to the Unity Asset Store.
It has a fix for the issue you reported and many other changes/fixes.

Update and check it out once released.
Thank you for your patience and understanding!