diff --git a/CHANGELOG.md b/CHANGELOG.md index 56cbe1e..abea1df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,10 @@ +# 2.3.0 + +- Remove `Promise.spawn` from the public API. +- `Promise.async` still inherits the behavior from `Promise.spawn`. +- `Promise.async` now wraps the callback in `pcall` and rejects if an error occurred. +- `Promise.new` has now has an explicit error message when attempting to yield inside of it. + # 2.2.0 - `Promise.promisify` now uses `coroutine.wrap` instead of `Promise.spawn` diff --git a/lib/README.md b/lib/README.md index 1cc638b..892bf42 100644 --- a/lib/README.md +++ b/lib/README.md @@ -76,12 +76,12 @@ docs: - name: async tags: [ 'constructor' ] desc: | - The same as [[Promise.new]], except it implicitly uses [[Promise.spawn]] internally. Use this if you want to yield inside your Promise body. + The same as [[Promise.new]], except it allows yielding. Use this if you want to yield inside your Promise body. If your Promise body does not need to yield, such as when attaching `resolve` to an event listener, you should use [[Promise.new]] instead. ::: tip - Promises created with [[Promise.async]] don't begin executing until the next `RunService.Heartbeat` event, even if the executor function doesn't yield itself. Learn more + Promises created with [[Promise.async]] don't begin executing until the next `RunService.Heartbeat` event, even if the executor function doesn't yield itself. This is to ensure that Promises produced from a function are either always synchronous or always asynchronous. Learn more ::: ```lua @@ -192,16 +192,6 @@ docs: returns: - type: boolean desc: "`true` if the given `object` is a Promise." - - name: spawn - desc: Spawns a thread with predictable timing. The callback will be called on the next `RunService.Heartbeat` event. - static: true - params: - - name: callback - type: - kind: function - params: "...: ...any?" - - name: "..." - type: "...any?" # Instance methods - name: andThen diff --git a/lib/Usage.md b/lib/Usage.md index 2515a54..3615dad 100644 --- a/lib/Usage.md +++ b/lib/Usage.md @@ -68,7 +68,7 @@ end) You must observe the result of a Promise, either with `catch` or `finally`, otherwise an unhandled Promise rejection warning will be printed to the console. -If an error occurs while executing the Promise body, the Promise will be rejected automatically with the error text if it's in a synchronous Promise. Otherwise, the error won't be caught. +If an error occurs while executing the Promise body, the Promise will be rejected automatically with the error text and a trace back. ## Chaining @@ -122,31 +122,20 @@ Promise.async(function(resolve) end) ``` -`Promise.async` uses `Promise.new` internally, except it wraps the Promise executor with . +`Promise.async` uses `Promise.new` internally, except it allows yielding while `Promise.new` does not. -`Promise.async` is sugar for: +`Promise.async` attaches a one-time listener to the next `RunService.Heartbeat` event to fire off the rest of your Promise executor, ensuring it always waits at least one step. -```lua -Promise.new(function(resolve, reject, onCancel) - Promise.spawn(function() - -- ... - end) -end) -``` - -### Promise.spawn -`Promise.spawn` attaches a one-time listener to the next `RunService.Heartbeat` event to fire off the rest of your Promise executor, ensuring it always waits at least one step. - -The reason `Promise.spawn` includes this wait time is to ensure that your Promises have consistent timing. Otherwise, your Promise would run synchronously up to the first yield, and asynchronously afterwards. This can often lead to undesirable results. Additionally, Promises that never yield can resolve completely synchronously, and this can lead to predictable, but often unexpected timing issues. Thus, we use `Promise.spawn` so there is always a guaranteed yield before execution. +The reason `Promise.async` includes this wait time is to ensure that your Promises have consistent timing. Otherwise, your Promise would run synchronously up to the first yield, and asynchronously afterwards. This can often lead to undesirable results. Additionally, Promise executors that only sometimes yield can lead to unexpected timing issues. Thus, we use `Promise.async` so there is always a guaranteed yield before execution. ::: danger Don't use regular spawn -`spawn` might seem like a tempting alternative to `Promise.spawn` here, but you should **never** use it! +Using `spawn` inside `Promise.new` might seem like a tempting alternative to `Promise.async` here, but you should **never** use it! `spawn` (and `wait`, for that matter) do not resume threads at a consistent interval. If Roblox has resumed too many threads in a single Lua step, it will begin throttling and your thread that was meant to be resumed on the next frame could actually be resumed several seconds later. The unexpected delay caused by this behavior will cause cascading timing issues in your game and could lead to some potentially ugly bugs. ::: ### When to use `Promise.new` -In some cases, it is desirable for a Promise to execute completely synchronously. If you don't need to yield in your Promise executor, and you are aware of the timing implications of a completely synchronous Promise, then it is acceptable to use `Promise.new`. +In some cases, it is desirable for a Promise to execute completely synchronously. If you don't need to yield in your Promise executor, then you should use `Promise.new`. For example, an example of a situation where it might be appropriate to use Promise.new is when resolving after an event is fired. @@ -168,7 +157,7 @@ If you attach a `:andThen` or `:catch` handler to a Promise after it's been canc ::: warning If you cancel a Promise immediately after creating it without yielding in between, the fate of the Promise is dependent on if the Promise handler yields or not. If the Promise handler resolves without yielding, then the Promise will already be settled by the time you are able to cancel it, thus any consumers of the Promise will have already been called and cancellation is not possible. -If the Promise does yield, then cancelling it immediately *will* prevent its resolution. This is always the case when using `Promise.async`/`Promise.spawn`. +If the Promise does yield, then cancelling it immediately *will* prevent its resolution. This is always the case when using `Promise.async`. ::: Attempting to cancel an already-settled Promise is ignored. diff --git a/lib/init.lua b/lib/init.lua index 3c32a17..3fa71d4 100644 --- a/lib/init.lua +++ b/lib/init.lua @@ -3,7 +3,6 @@ ]] local RunService = game:GetService("RunService") -local PROMISE_DEBUG = false --[[ Packs a number of arguments into a table and returns its length. @@ -16,30 +15,24 @@ local function pack(...) return len, { ... } end ---[[ - wpcallPacked is a version of xpcall that: - * Returns the length of the result first - * Returns the result packed into a table - * Passes extra arguments through to the passed function; xpcall doesn't - * Issues a warning if PROMISE_DEBUG is enabled -]] -local function wpcallPacked(f, ...) - local argsLength, args = pack(...) +local function packResult(...) + local result = (...) - local body = function() - return f(unpack(args, 1, argsLength)) + return result, pack(select(2, ...)) +end + +local function ppcall(callback, ...) + local co = coroutine.create(callback) + + local ok, len, result = packResult(coroutine.resume(co, ...)) + + if ok and coroutine.status(co) ~= "dead" then + error("Yielding inside Promise.new is not allowed! Use Promise.async or create a new thread in the Promise executor!", 2) + elseif not ok then + result[1] = debug.traceback(result[1], 2) end - local resultLength, result = pack(xpcall(body, debug.traceback)) - - -- If promise debugging is on, warn whenever a pcall fails. - -- This is useful for debugging issues within the Promise implementation - -- itself. - if PROMISE_DEBUG and not result[1] then - warn(result[2]) - end - - return resultLength, result + return ok, len, result end --[[ @@ -48,13 +41,12 @@ end ]] local function createAdvancer(callback, resolve, reject) return function(...) - local resultLength, result = wpcallPacked(callback, ...) - local ok = result[1] + local ok, resultLength, result = ppcall(callback, ...) if ok then - resolve(unpack(result, 2, resultLength)) + resolve(unpack(result, 1, resultLength)) else - reject(unpack(result, 2, resultLength)) + reject(unpack(result, 1, resultLength)) end end end @@ -183,9 +175,8 @@ function Promise.new(callback, parent) return self._status == Promise.Status.Cancelled end - local _, result = wpcallPacked(callback, resolve, reject, onCancel) - local ok = result[1] - local err = result[2] + local ok, _, result = ppcall(callback, resolve, reject, onCancel) + local err = result[1] if not ok and self._status == Promise.Status.Started then reject(err) @@ -195,25 +186,20 @@ function Promise.new(callback, parent) end --[[ - Promise.new, except Promise.spawn is implicit. + Promise.new, except pcall on a new thread is automatic. ]] function Promise.async(callback) - return Promise.new(function(...) - return Promise.spawn(callback, ...) - end) -end + local traceback = debug.traceback() + return Promise.new(function(resolve, reject, onCancel) + local connection + connection = RunService.Heartbeat:Connect(function() + connection:Disconnect() + local ok, err = pcall(callback, resolve, reject, onCancel) ---[[ - Spawns a thread with predictable timing. -]] -function Promise.spawn(callback, ...) - local args = { ... } - local length = select("#", ...) - - local connection - connection = RunService.Heartbeat:Connect(function() - connection:Disconnect() - callback(unpack(args, 1, length)) + if not ok then + reject(err .. "\n" .. traceback) + end + end) end) end @@ -661,8 +647,10 @@ function Promise.prototype:_finalize() end -- Allow family to be buried - self._parent = nil - self._consumers = nil + if not Promise.TEST then + self._parent = nil + self._consumers = nil + end end return Promise \ No newline at end of file diff --git a/lib/init.spec.lua b/lib/init.spec.lua index 1efc15c..3fdeb4f 100644 --- a/lib/init.spec.lua +++ b/lib/init.spec.lua @@ -1,5 +1,6 @@ return function() local Promise = require(script.Parent) + Promise.TEST = true local function pack(...) local len = select("#", ...) @@ -72,7 +73,6 @@ return function() expect(promise:getStatus()).to.equal(Promise.Status.Rejected) expect(promise._values[1]:find("hahah")).to.be.ok() - -- Loosely check for the pieces of the stack trace we expect expect(promise._values[1]:find("init.spec")).to.be.ok() expect(promise._values[1]:find("new")).to.be.ok()