Roblox/rodux

Cannot yield on .Changed

Quenty opened this issue · 3 comments

Quenty commented

Instead of preventing yielding, the Signal implementation should use Bindable events, letting Roblox's task scheduling pick up the slack.

See https://github.com/Quenty/NevermoreEngine/blob/version2/Modules/Events/Signal.lua for an implementation that accepts yielding.

LPGhatguy commented

We have an implementation of Signal from the core scripts that uses BinableEvent internally that I could have adapted.

There were a few issues that led to me avoiding it:

  • Yielding caused a lot of bugs working on iOS Lua chat
    • We decided that any attempts to yield are bugs.
  • I wasn't comfortable with the ordering guarantees of BindableEvent
    • Right now, it fires in FILO order, this signal is FIFO
    • I'm not sure if BindableEvent's order will be stable in the future
  • Lemur, the Roblox emulation layer that lets us run unit tests on Travis CI, doesn't implement an event loop yet, so BindableEvent isn't implemented.
zeux commented

I'm not sure if BindableEvent's order will be stable in the future

No promises on that front - we were discussing alternatives for internal signal implementation and one day we might do FIFO. We won't change this lightly of course (only if there are large perf/memory benefits) but it might happen.

LPGhatguy commented

I'm going to go ahead and close this issue for now, but if something else comes up (like another feature request to switch to a different signal implementation, fix up NoYield, etc) feel free to open up a new one.