setState locking throws confusing errors from event listeners

AmaranthineCodices commented 6 years ago

The functionality that throws if setState is used in lifecycle hooks is, in its present implementation, causing some issues with certain event listeners. Sometimes, property change listeners can be invoked synchronously during the render - if they call setState within the synchronous part of the handler, they will throw, because the component is still rendering.

Change events are as synchronous as possible

Change events are executed synchronously where possible. The code will execute in the current context up until it yields; the code after the yield will be executed later. As an example, this code (in a Frame):

local frame = script.Parent

frame:GetPropertyChangedSignal("AbsolutePosition"):Connect(function()
    print("Immediate")
    spawn(function()
        print("Yielded")
    end)
end)

print("Before")
frame.Position = UDim2.new(0, 1, 0, 0)
print("After")

prints this output:

Before
Immediate
After
Immediate
Yielded (x2)

Crucially, note that the first Immediate is printed between Before and After.

Why this is an issue in Roact

The current setState locking code is very, very simplistic. Each stateful component stores a boolean that determines whether setting state is allowed at the moment. This flag is set to false whenever the component starts re-rendering, and reset to true when the rendering is over.

The problem comes into play when you have a change event that fires synchronously, as in the scenario detailed above, and you call setState inside that change handler. As an example, this code will throw immediately after being run:

local Roact = require(game.ReplicatedStorage.Roact)
local e = Roact.createElement

local DemoComponent = Roact.PureComponent:extend("Demo")

function DemoComponent:init()
    self.state = {
        value = 1
    }
end

function DemoComponent:render()
    return e("ScreenGui", {}, {
        Frame = e("Frame", {
            Position = UDim2.new(0.5, 0, 0.5, 0),
            [Roact.Event.Changed] = function(rbx, property)
                if property ~= "AbsolutePosition" then return end

                self:setState({
                    value = math.random()
                })
            end
        })
    })
end

Roact.reify(e(DemoComponent), game.Players.LocalPlayer:WaitForChild("PlayerGui"))

This is the error thrown:

17:28:20.293 - setState cannot be used currently, are you calling setState from any of:
17:28:20.294 - * the willUpdate or willUnmount lifecycle hooks
17:28:20.294 - * the init function
17:28:20.295 - * the render function
17:28:20.295 - * the shouldUpdate function
17:28:20.295 - Stack Begin
17:28:20.296 - Script 'ReplicatedStorage.Roact.Component', Line 128 - method setState
17:28:20.297 - Script 'Players.ProlificLexicon.PlayerScripts.LocalScript', Line 19 - upvalue method
17:28:20.297 - Script 'ReplicatedStorage.Roact.SingleEventManager', Line 27
17:28:20.298 - Stack End

What happens is the event is connected, then the position is immediately changed. This fires the event, which executes synchronously during the render process. The event listener then calls setState, throwing an error, as designed.

Possible resolutions

There are a couple possible ways to fix this in Roact alone, though users of refs will still suffer from this problem in all of them:

spawn all event listeners in a new thread, delaying them by a frame and forcing them to wait until the rendering is completed
When rendering is started, suspend all event connections and discard all firings
When rendering is started, defer execution of event listeners until after rendering is done
Asynchronous rendering (#18)

`spawn` listeners in a new thread

This is the worst of the options overall. It will immediately resolve the problem, but it also prevents Roblox from re-using event threads. It also yields, which is messy.

Suspend listener invocation

This just turns off events completely while the component is rendering. This has obvious implications for data loss.

Defer listener invocation

This postpones event listener invocation until after rendering is completed, when setState can be safely called again. This has a bunch of possible pitfalls, however. The largest one on my mind is: What happens if the information in the event is stale by the time it's re-rendered? For example, if we fire an event, which will pass rbx to the listener, and then do something to make that rbx reference invalid, how do we resolve that?

Asynchronous rendering

This would fix the issue by disconnecting setState from rendering - calling setState with asynchronous rendering would not lead to a synchronous render, it would lead to a render at some point in the future. Calling setState within the render process (as would happen from a synchronously-invoked change listener) would cause another render to happen at some point in the future.

I wonder if we could suspend event listener invocation during rendering, at least for Roact-related events. That would be possible to do in SingleEventManager. I originally rejected this because of the potential for data loss, but after thinking about it further I'm not sure it's a big issue. Thoughts?

LPGhatguy commented 6 years ago

I think that could be a really good idea to at least explore. It would prevent many mistakes. I'll start up a new issue for it.

Roblox / roact