Maid::GivePromise() may leak resources

Aperta
  • Qualcosa non va?
#391 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub

I maintainer di solito rispondono entro 1 giorno

Nessuno ha ancora preso questa issue.

Valutazione

Difficoltà
2/5
Tempo stimato
1-3 ore
Idoneità per principianti
42/100
Tipo di issue
Documentazione
Chiarezza
Abbastanza chiara
Stato di attività
Ferma
Stack tecnologico
lua
Ambito
documentation

Direzione di ricerca

Inizia dalla documentazione di Maid::GivePromise e confrontane la formulazione con il comportamento di GiveTask descritto nell’issue. Chiarisci che GivePromise restituisce una promise racchiusa e annulla il wrapper durante la pulizia di maid senza assumere la proprietà della promise originale; completato quando gli esempi e la precisazione sulla proprietà corrispondono accuratamente al comportamento descritto.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Descrizione

bug

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.

Lingua principale
Lua
Stelle
632
Fork
148
Merge medio
15h 41m
PR unite (30g)
13

Preparare l'ambiente

Apri in Codespaces

Avvia il container di sviluppo del progetto nel browser, con il tuo account GitHub.

Come iniziare

  1. Leggi tutta la issue e poi la guida ai contributi del progetto.
  2. Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
  3. Fai un fork del repository e lavora su un branch.
  4. Apri una pull request che faccia riferimento al numero della issue.

Altre issue di Quenty/NevermoreEngine

Tutte le issue di Quenty/NevermoreEngine

Issue simili

Altre issue su Lua

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.