Maid::GivePromise() may leak resources

[OttoHatt]

In the docs, Maid::GivePromise says it 'Gives a promise to the maid for clean'. Although it returns a promise (rather than a taskid), I assumed this method would behave identically to Maid::GiveTask; you give a resource to a maid, and that maid takes ownership by handling its destruction.

-- Identical?
local promise = Promise.new()
maid:GiveTask(promise)
promise:Then(function() end)

-- Identical?
maid:GivePromise(Promise.new()):Then(function() end)

Instead, I realised that Maid::GivePromise actually wraps the given promise in a new promise, returns that wrapped promise, and will cancel the wrapped promise on cleanup. This means that destroying the maid will cancel the wrapped promise, breaking the chain - but the original promise may still be left pending.

This is a problem because the still-living original promise could may be keeping resources around beyond their usefulness.

local function promiseTimeout()
	local promise = Promise.new()

	local maid = Maid.new()
	promise:Finally(function()
		maid:Destroy()
	end)

	maid:GiveTask(cancellableDelay(1, function()
		print("Promise alive - trying to resolve.")
		promise:Resolve()
	end))

	return promise
end

local maid = Maid.new()

-- This example prints 'Promise alive - trying to resolve.' in the console.
-- Because we assumed the given promise is owned (and destroyed) by the maid, we just leaked a cancellableDelay.
-- What if this delay was longer? Could we accumulate unused delays?
maid:GivePromise(promiseTimeout()):Then(function()
	print("Timeout!")
end)
maid:Destroy()

As a counter-example, if we used Maid::GiveTask in the format originally described...

-- ...
-- Nothing ever prints to the console.
-- This is safe. Nothing leaked.
-- :GiveTask() takes ownership of the resource.
local promise = promiseTimeout()
maid:GiveTask(promise)
promise:Then(function()
	print("Timeout!")
end)
maid:Destroy()
-- ...

The typical use case of a promise is wrapping something intangible, i.e. HttpService::RequestAsync, promiseBoundClass, a BindableEvent. It's easy to miss that these resources have been leaked.

Here's a real-world example where failing to clean up the given promise is bad.

-- If we give this promise to a maid via `::GivePromise`, this part remains until a player touches it.
-- Even though nobody is listening!
local function promiseCharacterTouchTrigger()
	local promise = Promise.new()

	local maid = Maid.new()
	promise:Finally(function()
		maid:Destroy()
	end)

	local part = Instance.new("Part")
	part.Anchored = true
	part.Archivable = false
	part.CanCollide = false
	part.Transparency = 0.5
	part.CanTouch = true
	part.Size = Vector3.one * 8
	part.Parent = workspace
	maid:GiveTask(part.Touched:Connect(function()
		promise:Resolve()
	end))
	maid:GiveTask(part)

	return promise
end

The maid method is useful when you want cache a promise and pass it to many listeners.

function Class:PromiseDataStore()
	return self._dataStorePromise
end

Consumers taking ownership of that unwrapped promise would be really bad! They'd indirectly mutate the service on cleanup, causing a race condition.

However I think most users create a fresh promise per consumer; both Janitor and Trove take ownership and destroy the original promise, rather than just break the chain like Maid does. The behaviour of Maid::GivePromise can't be changed, but the docs should reflect the subtlety of its usage.