Maid::GivePromise() may leak resources

Abierto
  • ¿Algo no está bien?
#391 0 comentarios 0 reacciones 0 asignados Ver en GitHub

Los mantenedores suelen responder en 1 día

Nadie ha tomado este issue todavía.

Evaluación

Dificultad
2/5
Tiempo estimado
1-3 horas
Aptitud para principiantes
42/100
Tipo de issue
Documentación
Claridad
Bastante claro
Estado de actividad
Estancado
Stack tecnológico
lua
Área
documentation

Línea de trabajo

Comienza por la documentación de Maid::GivePromise y compara su redacción con el comportamiento de GiveTask descrito en el issue. Deja claro que GivePromise devuelve una promise envuelta y cancela el wrapper durante la limpieza de maid sin asumir la propiedad de la promise original; se considera terminado cuando los ejemplos y la aclaración sobre la propiedad coincidan exactamente con el comportamiento descrito.

Escrito por el modelo de indexación a partir del texto del issue.

Descripción

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.

Lenguaje dominante
Lua
Estrellas
632
Forks
148
Merge medio
15 h 41 min
PR fusionados (30 d)
13

Preparar el entorno

Abrir en Codespaces

Inicia el contenedor de desarrollo del proyecto en tu navegador, con tu propia cuenta de GitHub.

Primeros pasos

  1. Lee el issue completo y luego la guía de contribución del proyecto.
  2. Comenta en el issue que vas a ocuparte — evita que dos personas hagan lo mismo.
  3. Haz un fork del repositorio y trabaja en una rama.
  4. Abre un pull request que haga referencia al número del issue.

Más de Quenty/NevermoreEngine

Todos los issues de Quenty/NevermoreEngine

Issues similares

Más issues de Lua

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.