From 4d649ba92d309e7e905903666637f266812632a7 Mon Sep 17 00:00:00 2001
From: Guldoman <giulio.lettieri@gmail.com>
Date: Wed, 5 Jun 2024 14:58:19 +0200
Subject: [PATCH] fix: avoid iterating over a changing table in `run_threads`
 (#1794)

* fix: avoid iterating over a changing table in `run_threads`

This is done to avoid iterating over a table that can change in the meantime.
More precisely the issue appears if a thread is removed from the table, we yield early from `run_threads` because we reached the end of the frame, and a new thread is added before the next iteration.

For example:
```lua
local lost_time = false
core.add_thread(function()
  -- force early yield
  local t0 = system.get_time()
  while system.get_time() - t0 < .1 do end
  lost_time = true
end, "a")

local step = core.step
function core.step()
  if lost_time then
    -- add a new thread while run_threads hasn't finished iterating
    core.add_thread(function()end, "a1")
    lost_time = false
  end
  return step()
end
```
would crash with `invalid key to 'next'`.

* fix: only run coroutine if it wasn't removed

* fix: don't handle `core.threads` table as an array

This caused some entries to be skipped or even removed erroneously.
---
 data/core/init.lua | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/data/core/init.lua b/data/core/init.lua
index 884cfef8..910c24a3 100644
--- a/data/core/init.lua
+++ b/data/core/init.lua
@@ -1127,8 +1127,14 @@ function core.show_title_bar(show)
 end
 
 
+local thread_counter = 0
 function core.add_thread(f, weak_ref, ...)
-  local key = weak_ref or #core.threads + 1
+  local key = weak_ref
+  if not key then
+    thread_counter = thread_counter + 1
+    key = thread_counter
+  end
+  assert(core.threads[key] == nil, "Duplicate thread reference")
   local args = {...}
   local fn = function() return core.try(f, table.unpack(args)) end
   core.threads[key] = { cr = coroutine.create(fn), wake = 0 }
@@ -1402,16 +1408,20 @@ local run_threads = coroutine.wrap(function()
     local max_time = 1 / config.fps - 0.004
     local minimal_time_to_wake = math.huge
 
+    local threads = {}
+    -- We modify core.threads while iterating, both by removing dead threads,
+    -- and by potentially adding more threads while we yielded early,
+    -- so we need to extract the threads list and iterate over that instead.
     for k, thread in pairs(core.threads) do
-      -- run thread
-      if thread.wake < system.get_time() then
+      threads[k] = thread
+    end
+
+    for k, thread in pairs(threads) do
+      -- Run thread if it wasn't deleted externally and it's time to resume it
+      if core.threads[k] and thread.wake < system.get_time() then
         local _, wait = assert(coroutine.resume(thread.cr))
         if coroutine.status(thread.cr) == "dead" then
-          if type(k) == "number" then
-            table.remove(core.threads, k)
-          else
-            core.threads[k] = nil
-          end
+          core.threads[k] = nil
         else
           wait = wait or (1/30)
           thread.wake = system.get_time() + wait