Explain a memory leak

I was informed on my last post that this has a memory leak:

ClickDetector.MouseHoverEnter:Connect(function()
	warn("Hover")
	UserInputService.InputBegan:Connect(function(input)
		if input.KeyCode == Enum.KeyCode.E then
			if Holding == false then
				Holding = true
				warn("Holding")
				while Holding do
					RunService.RenderStepped:Connect(function()
						Part.CFrame = Part.CFrame + Part.CFrame.LookVector * Vector3.new(0, 0, 5)
					end)
				end
			end
		end
	end)
end)

I was wondering if anyone could explain exactly what a memory leak is and what I can do to fix them.

3 Likes

It’s here:

A memory leak happens when you don’t clear the memory you have used. This is almost always prevented using methods like garbage collection and you don’t need to care about it; however, in your case, the garbage collector can’t do anything since it looks like intended behavior to it.

You keep connecting a new function created every time. This is the memory leak.

Plus, Holding will never change to false, which turns that while loop into an infinite loop.

Not only that, there is a second (but much smaller) memory leak here:

I can’t recommend a fix to the latter without knowing what you want to do (it’s still pretty important though), but for the former and more important problem, you can define a variable outside the functions:
local connection
get rid of the while loop and then, you can assign to the connection like this:

connection = RunService.RenderStepped:Connect(function()
	Part.CFrame = Part.CFrame + Part.CFrame.LookVector * Vector3.new(0, 0, 5)
end)

When you don’t want to listen to the connection, do connection:Disconnect().

This was also how I used to view Events and Connections a while ago. Remember that once you connect to an event, the function will be called every time the event fires from that point on, until you disconnect it. It’s not something you have to ā€œremindā€ its existence to.

10 Likes

I’m making a system where you can pick up objects. I feel the need to have a while loop because I’m trying to check whether the object is currently being held or not. I did have an else statement that turned Holding to false but I guess I removed it before posting this for some reason.

I’m assuming you hold/put down using ā€˜e’ key. In which case you could just connect/disconnect the runservice function based on InputBegan/Ended events.

In that case, you need to change the code to this:


--define the variable Holding here if you haven't done it already:
local Holding = false

UserInputService.InputBegan:Connect(function(input)
	if input.KeyCode == Enum.KeyCode.E then
		Holding = true
	end
end)

UserInputService.InputEnded:Connect(function(input)
	if input.KeyCode == Enum.KeyCode.E then
		Holding = false
	end
end)

ClickDetector.MouseHoverEnter:Connect(function()
	while true do
		RunService.RenderStepped:Wait()
		if Holding then
			Part.CFrame = Part.CFrame + Part.CFrame.LookVector * Vector3.new(0, 0, 5)
		end
	end
end)

Once you connect to an event, it will work everytime the event is fired, forever. That’s what I changed, I got rid of the connections made multiple times since you only need one and it will work.

This code changes the variable Holding when you start/stop pressing E. After that, we connect to MouseHoverEnter, and then we’ve got an infinite loop. In that infinite loop, we wait until the RenderStepped event fires. When it does, and if we’re holding the thing, we change the part’s position.

If you ever need to connect to something in a connection itself, make sure that it’s necessary, and make sure that you’ll disconnect later. If none of these are the case, you’re probably doing something wrong.

By the way, from your previous question about the same script, you seem to be using a server script, and you really shouldn’t (it simply will not work). The server is unrelated to the clients, and the clients hold their own GUIs, RemoteEvent firing code, etc.

While this fix I made will seem to work, it’s not going to replicate to the server. You should use a RemoteEvent.

3 Likes

you don’t need all of that,
you could just do

local Holding
ClickDetector.MouseHoverEnter:Connect(function()
	warn("Hover")
	UserInputService.InputBegan:Connect(function(input)
		if input.KeyCode == Enum.KeyCode.E then
				Holding = true
				warn("Holding")
				while Holding do
                wait()
			    Part.CFrame = Part.CFrame + Part.CFrame.LookVector * Vector3.new(0, 0, 5)
			end
		end
	end)
end)

UserInputService.InputEnded:Connect(function(input)
		if input.KeyCode == Enum.KeyCode.E then
			Holding = true
end
end)

Your code also won’t work. The problem is that this part:

creates an infinite loop in both your code and the original. There is a way of leaking memory as well since you don’t disconnect InputBegan. I see that you just added a wait, but what that doesn’t change the fact that we will be modifying the Part’s position forever.

1 Like

ik its late reply, but this post didnt got an answer, so here it is.
the while loop is running continuously without any delay or wait.

all u need was task.wait (its better than wait).
and there is no need to use renderstep under a loop that is already running.

2 methods

while Holding do
	 task.wait()
     Part.CFrame = Part.CFrame + Part.CFrame.LookVector * Vector3.new(0, 0, 5)	
end

or

RunService.RenderStepped:Connect(function()
	Part.CFrame = Part.CFrame + Part.CFrame.LookVector * Vector3.new(0, 0, 5)
end)
1 Like