setState locking throws confusing errors from event listeners

[AmaranthineCodices]

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 (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.

See also

• Give useful error messages when calling setState in the wrong places #17: Original issue that prompted this behavior to be introduced
• setState sanity checking #23: PR that added this behavior
• More setState checks #26: Add-on to setState sanity checking #23.

[LPGhatguy]

Good write-up!

I don't think I like many of these solutions, even the asynchronous one. If a render is being caused by a render (a cascading render), that's most likely a bug.

Even once asynchronous rendering lands, we should catch these cases and warn, and recommend that users wrap changed events in a spawn to avoid triggering cascading renders.

It might be possible to special-case both Changed and GetPropertyChangedSignal() signals to show a better message, since they're the only ones that should be triggerable in the middle of a render.

[AmaranthineCodices]

So the recommended solution here is wrapping change events that call setState in spawn blocks (or yielding before calling setState)?

[LPGhatguy]

Roblox needs some sort of fastSpawn to make that actually workable I think

[LPGhatguy]

I think we need to update the setState locking code to throw specific error messages based on the situation, instead of a blanket error message.

It's been waaaay too confusing for everyone that hits it.

[LPGhatguy]

It's possible this was partly fixed by #67. It doesn't quite solve the crux of the issue (Changed is evil) but should make the problem much more debuggable.

[AmaranthineCodices]

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]

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.