Unsafe handling of values in module script table? send help

Hey there!

Today is day 4 of trying to fix a dupe glitch in my game so I am reaching out for help.

Essentially, I now believe that the dupe is originating from unsafe handling of variables in a priority queue. Specifically I think that the nested while loop below sometimes starts another iteration over itself before queue:pop() has safely removed the top element from the queue. BTW the dupe is happening when processQueue() is being called and a lot of items are being added to the queue.

I am not super familiar with coroutines so please tell me if the system below is jank as heck and I should be using wrap() or something like that. Any help is appreciated!

local serverFunctions = require(game.ServerScriptService.modules.serverFunctions)
local inventoryRequests = require(game.ServerScriptService.modules.inventoryRequests)
local queue = inventoryRequests.PriorityQueue

local inventoryEvent = game.ReplicatedStorage.events.inventoryEvent

local processor = {}

local function job()
	warn("THREAD CREATED")
	while true do
		while #queue.heap > 0 do
			local request = queue:pop()
			local success, changeTable = serverFunctions.plrToStorage(request)
			inventoryEvent:FireClient(request[1], success, changeTable)
		end

		coroutine.yield()
	end
end

local processTask = nil

function processor.processQueue()
	if not processTask or coroutine.status(processTask) == "dead" then
		warn("REVIVING")
		processTask = coroutine.create(job)
	end
	
	if coroutine.status(processTask) == "running" then
		warn("Processor queue is already running.")
		return
	end
	
	coroutine.resume(processTask)
end

return processor

1 Like

Does it show in console multiple threads being made, or just one?

2 Likes

I apologize I should have specified what I seeing in console. The console only ever shows THREAD CREATED 1 time. I should also add that the dupe is rare and only happens probably every 25 iterations of the while loop? The whole thing is kinda strange and I am at a loss for what to do. THis is the only place that could be the issue.

I kind of just hoped somebody had seen something like this before cuz i t seems to be like a Studio issue rather than a code one (although my code prob isn’t the best either).

1 Like

Hi! in your processQueue function, why are you checking if it’s running and returning if it is. Wouldn’t it be more efficient to check if the coroutine.status is suspended then resuming it from there?

To address your problem I haven’t had many duping issues myself, but in your InventoryEvent, on the client side, are you checking if success is true? if it is, then you can create a dictionary on the client and assign them random ids. That way, you could check if the ID has already been process on the client once the new data gets to them.

2 Likes

Can we see the implementation for the priority queue? Also, are you 100% sure the dupe is happening from the processing and not other code mistakenly adding a dupe to the queue? Have you tried logging the additions to the queue and logging what gets processed in order to find the discrepancy?

I don’t really see anything wrong with the code you’ve posted. The one thing I like to do in cases like this is move all the things I’m going to process into a different table. That way, if my processing code yields for any reason, there’s no risk that the table I’m iterating over will be modified mid-loop I doubt it would fix anything here, but it’s just a way for me to keep the processing separate from the addition of requests.

1 Like
  1. Not your fault as it is poorly explained, but calling coroutine.status on a “running” thread outside of that said thread will return suspended (unfortunately).
  2. There is no need to use coroutines in this case. Because of the issue above, calling coroutine.resume on an already running thread will have unintended consequences. You can achieve most of, if not everything, without the use of coroutine.create/resume and coroutine.wrap. In your case, just create a new thread with task.spawn whenever you call processQueue, and drop the request when it’s already running.
local jobThread: thread? = nil

function processor.processQueue(): ()
	if jobThread == nil and #queue.heap > 0 then
		print("Started job thread")
		
		jobThread = task.spawn(function(): ()
			while #queue.heap > 0 do
				local request = queue:pop()
				local success, changeTable = serverFunctions.plrToStorage(request)
				inventoryEvent:FireClient(request[1], success, changeTable)
			end
			
			jobThread = nil
		end)
	else
		warn("Job thread is running")
	end
end

Can you elaborate on this? From what I understand, if you’re calling coroutine.status on a separate thread, then it’s impossible for that thread to be running and it must be either suspended, dead, or waiting on the yield of another thread. I don’t think there’s any weird behavior regarding that function.

1 Like
local thread: thread; thread = coroutine.create(function(): ()
	print(coroutine.running(), coroutine.status(coroutine.running())) -- thread: 0xa3f78377a1fa269f running
	print(thread, coroutine.status(thread)) -- thread: 0xa3f78377a1fa269f running
	
	task.wait(1)
end)

coroutine.resume(thread)
print(thread, coroutine.status(thread)) -- thread: 0xa3f78377a1fa269f suspended
task.wait(1)
print(thread, coroutine.status(thread)) -- thread: 0xa3f78377a1fa269f dead

Can you elaborate on this? Would genuinely like to know what you mean by this so I can correct myself.

Ah, alright I get what you mean.

1 Like

Hey guys!

So I found the problem. Turns out the issue was originating from the storage-to-plr logic instead of the plr-to-storage logic as I thought before. The reason I didn’t notice this is because I was only looking at the UI data on the client (which was totally idiotic looking back). So no wonder my attempts to change plr-to-storage didn’t do anything!

Needless to say, I explored all of your suggestions and they were what kept me motivated to eventually solve the bug. So even though none of them actually helped (because I didn’t give you relevant information to what happened to be the real problem), you all helped!

Thanks!

1 Like