ClientRemoteProperty doesn't update in edge case

[ambergamefam]

Fix: #157

Currently, there is an edge case where a RemoteProperty can initialize with an old value sent from the server, and never updates with the latest value. This is actually super common in cases where Knit properties are created and updated for a player as soon as they join the game, and has caused problems in my game.

This is the problematic line of code:

RbxUtil/modules/comm/Client/ClientRemoteProperty.lua

Line 92 in b91a47d

return Promise.fromEvent(self._rs, function(value)

Essentially, this is the the order of things going on that makes this line of code cause problems:

• Knit:Start() is called on the server
• A Knit service is called which initializes a remote property
• A player joins the game, and the property is initialized with a value for that player
• The server immediately changes the value of this property one or more times. This queues up 3 remote events to the client that will immediately be handled once connected
• Knit:Start() is called on the client
• A knit controller gets the service, which instantiates the ClientRemoteProperty
• The ClientRemoteProperty referencing the remote signal that has 3 events queued up
• OnReady() is called, and creates a promise that automatically connects to the signal. This causes the signal to emit 3 events since this is the first time the player has connected to this remote signal. The promise only picks up the first of these 3 events, then drops the remaining 2.
• After OnReady() completes with the first value received, the remote is connected to a second time, but this connection does not pick up the 2 queued values, dropping them entirely.

To fix this issue, the remote property either needs to initialize after the last queued-up event is processed, or it needs to queue up these changes, initialize with the first value, then emit change events for the next remote updates.

[Sleitnick]

@ambergamefam Do you by chance have some simple repro code I could run to test this out? I can't seem to reproduce the original issue on my end.

I do entirely agree with the queued events logic though. If the promise is just waiting for the first event to come through, then subsequent queued events will likely get dropped. So yeah, that's definitely a problem. Not sure why I'm unable to reproduce the issue though.

[ambergamefam]

I was able to create a repro. Looks like this only happens A) on Deferred signal behavior, and B) only in cases where data is coincidentally set for the player on the same frame where the player requests an initial value of the property from the server. In my case, since there were lots of services and properties in use, the chance of running into this issue for at least one part of the game was actually pretty high.

CommDeferredRepro.rbxl.zip

Source code is not rojo-managed in this case.

---

With my PR, this repro shows all values being replicated to the client, since the queued-up events are handled at once:

CommDeferredReproWithFix.rbxl.zip

Note that the value "5" is received twice since the initial value is sent to the player on the same frame that the value was assigned to 5.

[ambergamefam]

I will be setting my game back to "Immediate" signal behavior as a workaround for the time being, but if you could take a look at my fix (#157), I would greatly appreciate it. @Sleitnick

[Sleitnick]

@ambergamefam Yup, please note the pending comments on the PR

[Sleitnick]

Fixed in Comm v0.3.2, and Knit updated to v1.5.2 with the fix

[ambergamefam]

@Sleitnick There will still be an edge case if you don't create a closure when deferring the :Observe() callback
9520c61#r128355932

[Sleitnick]

What is the edge case? They should be functionally equivalent

[ambergamefam]

From the comment I linked:

This is incorrect. This needs to be:

        task.defer(function()
                observer(self._value)
        end)

If you call task.defer with the current value, it will emit that value in a deferred thread, regardless if the value changed between now and when the observer runs. An example of this is when the value changes multiple times when multiple client events get processed in serial.

For example:

• The client runs and asks the server the ID of their house.
• The server receives this request and returns the ID of their house. Coincidentally (or not), a property of their house changes twice.
• There are now 3 events queued up to the client: House ID, and then the two property changes
• The first event processes (house ID). After the client receives the house ID, they want to begin observing these properties. They connect a callback to the remote property. This defers a thread with an old value of this property.
• The client then processes the next two events, changing this property. The callback to :Observe() fires twice with these values
• The deferred thread resumes, with the old value as an argument.

[ambergamefam]

Essentially here's the difference:

local value = 1
task.defer(print, value)
value = 2
print(value)

Prints:

2
1

VS

local value = 1
task.defer(function()
    print(value)
end)
value = 2
print(value)

prints:

2
2