Unable to destroy datastore instances causing memory leak

Issue Type: Other
Impact: Moderate
Frequency: Constantly
Date First Experienced:
Date Last Experienced:

Reproduction Steps:
To reproduce this issue create a new datastore object DataStoreService:GetDataStore(name, scope) and attempt to delete it.

local DataStore = game:GetService("DataStoreService")
--Player joined
local userId = 30311117
local MyPlayersDataStore = DataStore:GetDataStore("Resources", userId)

--Player left
MyPlayersDataStore:Destroy()
--Error:  The Parent property of DataStoreService.Resources is locked

Expected Behavior:
I expect to be able to Destroy the instance to clean up memory on the server, or have GC clean it up.

Actual Behavior:
Joining a few different servers on my game and running print(#game:GetService("DataStoreService"):GetChildren()) shows that these instances exist permanently. Which increases server memory usage slowly for each player that joins the game.

Workaround:
Use one datastore object for all players instead of using a seperate one based on UserId.

3 Likes

Did you try creating a million or more of these and seeing if they are cleaned up automatically?

Sometimes a GC cycle won’t kick in on certain objects unless memory actually needs to be cleaned up. Keeping memory allocated or objects in the data model does not necessarily need to be a bad thing as long as high load will eventually rotate them out and not keep consuming memory until a crash happens. Cleaning up memory/objects can take time or reduce caching benefits, after all, so it might be intentional to keep them around until some thresholds are exceeded.

That would mean DataStore2 would have issues. However the use of multiple DataStores (and the use of scopes) is discouraged.

I’m not saying it shouldn’t be fixed, it should be fixed especially since DataStore2 with multiple developers using it, and with this would cause issues. But you should only be using 1 datastore for type of data.

Example:

DataStore: “PlayerData”, no scope.

Key: (userId from a player)

(Converting this later on is not even worth it)

This is not only a workaround, this is how Roblox wants you to use DataStores, they behave similarly to tables internally, so creating more datastores for them is more expensive and will soon turn mostly pointless. So I would recommend you use it like that on any newer projects if you can.

Also thanks for letting me know I can see what datastores I am using via :GetChildren() :P

Just tested this, managed to get it up to 4M datastores, before server crashed, 6.2GB memory usage.
Uncopylocked repro place

]

local DataStoreService = game:GetService("DataStoreService")
local RunService = game:GetService("RunService")
local Stats = game:GetService("Stats")

local DATA_STORES_PER_LOOP = 100

local i = 0
local t = os.clock()
while true do
	for j = 1, DATA_STORES_PER_LOOP do
		DataStoreService:GetDataStore(i + j)
	end
	i += DATA_STORES_PER_LOOP
	if i%1e4 == 0 then
		local instanceCount = #DataStoreService:GetChildren()
		local memory = math.floor(Stats:GetTotalMemoryUsageMb())
		local instanceMemory = math.floor(Stats:GetMemoryUsageMbForTag(Enum.DeveloperMemoryTag.Instances))
		
		print("Instance Count:", instanceCount, "Memory:", memory, "Instances Memory:", instanceMemory)
		if memory - memoryCheckpoint > 1000 then
			wait(60)
			memoryCheckpoint = memory
		end
	end
	if os.clock() - t > 1/30 then
		RunService.Heartbeat:Wait()
		t = os.clock()
	end
end

Edit: Added 60 sec wait for every GB memory increase in case creating instances doesn’t give GC enough CPU time to clean up. Same results.

3 Likes

I agree that UserId should be the key, however even the example on devhub uses UserId as scope.

It does true, how ever overall scopes are “deprecated”, it’s still maintained, so it’s not, but Roblox says it shouldn’t be used, and if you want to separate datastores like that, to use .."/"..ScopeNameHere on the datastore name instead. I think they MIGHT (or might not) be ACTUALLY deprecated soon. We’ll have to wait and see I guess;

Source?

Try not to present things as fact when you don’t have the source to back it up, this seems like a really egregrious assumption with no backing.

It’s way more likely that they aggregate the game ID, name and scope into some hashing key that is used to distribute the data over multiple data centers, and that it doesn’t matter all that much (compared to what you imply) when using many “stores”. It’s not likely they create a whole new “table” on the back-end database for every unique GetDataStore call permutation, that doesn’t scale at all. The object representation in the data model doesn’t necessarily represent the back-end.

It’s actually some they talked about in the datastore 1.1 beta, (applies to “old” DataStore as well), they gave some tips to datastore use and future proofing, and they scopes and multiple DataStores use is mostly discouraged.

(I don’t consider this private info as it isn’t about the beta in question, just something they said there) it’s very likely that would be specified when it launches in the GetDataStore page.

We do discourage creating large number of datastores. We suggest using few (~10) datastores for your games. A “PlayerDatastore” with “player id” as key is encouraged, instead of using player id as part of the datastore name.

We will look into the potential memory leak mentioned here.

4 Likes

This should be mentioned on the relevant devhub articles if that is the case (the order of ~10 part), seems like new information to me.