BindToClose issue fixes (#134)

* Removed comment that 'require' is a valid way to get DataStore2

* Fixes Issue #124 - "Memory leak per player"

* Fixes #125 - Use a BindableEvent instead of setting Player.Parent to nil

* Fixes Issue #126 - Handles potential BindToClose error

* Added additional info to BindToClose failure warning

Co-authored-by: boyned//Kampfkarren <[email protected]>

* Applied suggestions to change `booleanVal == false` to `not booleanVal`

Co-authored-by: boyned//Kampfkarren <[email protected]>

* Applied code review suggestions

* Removed unnecessary promise catch

* Added Promise.defer to replace spawn
Also added comment to prevent accidental changes to deferred code

Co-authored-by: boyned//Kampfkarren <[email protected]>

CHANGELOG.md

@@ -12,6 +12,7 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 - Fix `GetTable` not working appropriately when using combined data stores.
 - Fix `:IncrementAsync` not returning a Promise.
 - Fix `OnUpdate` being fired twice when using `Update` to update data.
+- Fix `BindToClose` memory leak, breaking other BindToClose scripts, and potential error.
 
 ## [1.3.0]
 - Added :GetAsync(), :GetTableAsync, and :IncrementAsync(), which are [promise](https://github.com/evaera/roblox-lua-promise) versions of their non-async counterparts.

DataStore2/init.lua

@@ -543,40 +543,59 @@ function DataStore2.__call(_, dataStoreName, player)
 
 	setmetatable(dataStore, DataStoreMetatable)
 
-	local event, fired = Instance.new("BindableEvent"), false
-
-	game:BindToClose(function()
-		if not fired then
-			spawn(function()
-				player.Parent = nil -- Forces AncestryChanged to fire and save the data
+	local saveFinishedEvent, isSaveFinished = Instance.new("BindableEvent"), false
+	local bindToCloseEvent = Instance.new("BindableEvent")
+
+	local bindToCloseCallback = function()
+		if not isSaveFinished then
+			-- Defer to avoid a race between connecting and firing "saveFinishedEvent"
+			Promise.defer(function()
+				bindToCloseEvent:Fire() -- Resolves the Promise.race to save the data
 			end)
 
-			event.Event:Wait()
+			saveFinishedEvent.Event:Wait()
 		end
 
 		local value = dataStore:Get(nil, true)
 
 		for _, bindToClose in ipairs(dataStore.bindToClose) do
 			bindToClose(player, value)
 		end
+	end
+
+	local success, errorMessage = pcall(function()
+		game:BindToClose(function()
+			if bindToCloseCallback == nil then
+				return
+			end
+
+			bindToCloseCallback()
+		end)
 	end)
+	if not success then
+		warn("DataStore2 could not BindToClose", errorMessage)
+	end
 
-	local playerLeavingConnection
-	playerLeavingConnection = player.AncestryChanged:Connect(function()
-		if player:IsDescendantOf(game) then return end
-		playerLeavingConnection:Disconnect()
+	Promise.race({
+		Promise.fromEvent(bindToCloseEvent.Event),
+		Promise.fromEvent(player.AncestryChanged, function()
+			return not player:IsDescendantOf(game)
+		end),
+	}):andThen(function()
 		dataStore:SaveAsync():andThen(function()
 			print("player left, saved", dataStoreName)
 		end):catch(function(error)
 			-- TODO: Something more elegant
 			warn("error when player left!", error)
 		end):finally(function()
-			event:Fire()
-			fired = true
+			isSaveFinished = true
+			saveFinishedEvent:Fire()
 		end)
 
-		delay(40, function() --Give a long delay for people who haven't figured out the cache :^(
+		--Give a long delay for people who haven't figured out the cache :^(
+		return Promise.delay(40):andThen(function() 
 			DataStoreCache[player] = nil
+			bindToCloseCallback = nil
 		end)
 	end)