hammerspoon: Coroutines don't work in Hammerspon

Admittedly this is a sort of band-aid over a framework that should probably (eventually) be refactored now that we’ve realized that one of the core assumptions that went into making it was mistaken, but I think it should work, once we’ve addressed all dispatch_* blocks that assume the persistence of skin and don’t bother setting it again…

I’ll keep you guys posted and let you know when a fork is ready for testing.

@asmagill I think I’m mostly concerned about all the state in C that will now have multiple Lua environments mutating it.

I agree that the only way to be sure is to try, so maybe if we have a branch with the modifications to LuaSkin and then we need to hammer the HS API from the coroutines and the main thread and see what happens.

If the limitation is only that you cannot call Hammerspoon API functions from coroutines, you may be able to “fix” the problem from the Lua side by implementing asymmetric coroutines on top of symmetric coroutines on top of asymmetric coroutines, placing a trampoline inside of your new fake coroutine.resume, and wrapping Hammerspoon API functions to make sure they are called from the main lua_thread even when the user tries to use them from inside a coroutine.

A sketch of what this would look like, on the library side, which I’ve called co_wrap.lua over here:

local select = select
local table_insert = table.insert
local table_remove = table.remove
-- utility to save multiple returns or varargs for later use
local NIL = {}
local function drop_one(x, ...)
  return ...
end
local save_exprs
save_exprs = function(return_first_x, ...)
  if return_first_x == 0 then
    local ret = {}
    for i=1,select("#", ...) do
      local item = select(i, ...)
      if item == nil then
        item = NIL
      end
      ret[i] = item
    end
    return ret
  end
  return select(1, ...), save_exprs(return_first_x-1, drop_one(...))
end
local unsave_exprs
unsave_exprs = function(t, idx, len)
  if not idx then
    idx = 0
    len = #t
  end
  if idx == len then
    return
  end
  idx = idx + 1
  local ret = t[idx]
  if ret == NIL then
    ret = nil
  end
  return ret, unsave_exprs(t, idx, len)
end

local co_stack = {}
local co_running = {}
local n_cos = 0
local real_yield = coroutine.yield
local real_resume = coroutine.resume
local real_status = coroutine.status
local real_create = coroutine.create
function coroutine.resume(co, ...)
  -- can't have the same coroutine running twice in our fake stack of coroutines 
  if co_running[co] then
    return false, "cannot resume non-suspended coroutine"
  end
  -- if we are called from the main thread, resume and trampoline
  if n_cos == 0 then
    n_cos = n_cos + 1
    co_stack[n_cos] = co
    co_running[co] = true
    local ok, op
    local stuff = save_exprs(0, co, ...)
    while true do
      local prev_co = stuff[1]
      ok, op, stuff = save_exprs(2, real_resume(unsave_exprs(stuff)))
      if ok and op == "resume" then
        -- trampoline and pass stuff to nested co
        n_cos = n_cos + 1
        co_stack[n_cos] = stuff[1]
        co_running[stuff[1]] = true
      elseif ok and (op == "yield" or op == "die") then
        -- coroutine yielded or died
        -- trampoline and pass stuff to parent co
        co_running[prev_co] = nil
        co_stack[n_cos] = nil
        n_cos = n_cos - 1
        if n_cos == 0 then
          -- parent co is topmost lua thread, so return
          return true, unsave_exprs(stuff)
        else
          table_insert(stuff, 1, true)
          table_insert(stuff, 1, co_stack[n_cos])
        end
      elseif ok and op == "call" then
        -- coroutine wants to call some function from the main thread
        -- get the results and pass them back to same co
        local func = table_remove(stuff, 1)
        stuff = save_exprs(0, func(unsave_exprs(stuff)))
        table_insert(stuff, 1, co_stack[n_cos])
      elseif not ok then
        -- coroutine errored
        -- trampoline and pass stuff to parent co
        co_running[prev_co] = nil
        co_stack[n_cos] = nil
        n_cos = n_cos - 1
        if n_cos == 0 then
          -- parent co is topmost lua thread, so return
          return false, op, unsave_exprs(stuff)
        else
          table_insert(stuff, 1, op)
          table_insert(stuff, 1, false)
          table_insert(stuff, 1, co_stack[n_cos])
        end
      else
        error("Fake coroutine lib: Not sure what happened")
      end
    end
  else
    -- tell the loop on the main thread that I'd like to resume pls
    return real_yield("resume", co, ...)
  end
end

function coroutine.yield(...)
  -- tell the loop on the main thread that I'd like to yield pls
  return real_yield("yield", ...)
end

function coroutine.status(co)
  if co_running[co] then
    return "running"
  else
    return real_status(co)
  end
end

function coroutine.create(f)
  return real_create(function(...)
    -- tell the loop on the main thread that I'd like to die pls
    return "die", f(...)
  end)
end

function coroutine.call_from_main_thread(f)
  return function(...)
    if n_cos == 0 then
      return f(...)
    else
      -- tell the loop on the main thread that I'd like to call pls
      return real_yield("call", f, ...)
    end
  end
end

--TODO: also reimplement coroutine.wrap

--coroutine.running should work fine as is.

On the user side:

require"co_wrap"
get_main_thread = coroutine.call_from_main_thread(coroutine.running)
get_my_thread = coroutine.running
a = coroutine.create(function()
    local cos = {}
    local function print_some_numbers(n)
      for i=1,n do print("inner", i, coroutine.yield(i*i)) end
      print(get_main_thread())
      print(get_my_thread())
    end
    for i=1,3 do
      cos[i] = coroutine.create(print_some_numbers)
    end
    local n = 1
    while true do
      for i=1, #cos do
        if coroutine.status(cos[i]) ~= "dead" then
          print("outer", coroutine.resume(cos[i],n))
          n = n + 1
        end
      end
      print(coroutine.yield())
    end
  end)

for i=1,5 do print(coroutine.resume(a)) end

This prints

outer	true	1
outer	true	1
outer	true	1
true

inner	1	4
thread: 0x7f901b800608	true
thread: 0x7f901b408c08	false
outer	true
inner	1	5
outer	true	4
inner	1	6
outer	true	4
true

inner	2	7
thread: 0x7f901b800608	true
thread: 0x7f901b408f88	false
outer	true
inner	2	8
outer	true	9
true

inner	3	9
thread: 0x7f901b800608	true
thread: 0x7f901b408928	false
outer	true
true

true

If you remove lines 8 and 9, then you can get output that doesn’t change based on the addresses of coroutines, and you can try running it with and without co_wrap. Over here it produces identical output with and without co_wrap on lua 5.3.5. This isn’t a guarantee that I haven’t written any bugs, of course.

Edit: While this may let users run code that uses coroutines, it won’t be as fast as you might like. If you’d like to go fast, I think the right play is to fix the problem for real and to use some version of luajit.

Ok, no, that’s stupid… we’d just be checking the saved state, which we already know is the main “thread”… let me think some more…

Callbacks should always run on the main lua_State (the lua docs call it a thread, which I’ve sometimes called a “lua thread” to be pedantic, but it’s really primarily a separate isolated lua stack) --the initial lua_State created when the lua engine was initialized – because they are a new code-block triggered by the external event, not the resuming of something a coder suspended.

That’s why I recommend creating the new method sharedWithState: and applying it only to functions and methods we add, but not within the callback/delegate/event code that handles externally triggered application events. If we go with the first option I outline above, the internal changes to LuaSkin are manageable and there is still only one instance required.

As to why its taken so long to find out… well, most of our users aren’t programmers per se, and those that are didn’t start with Lua… coroutines are a fundamental part of the language, and the primary answer to the fact that it’s by default single threaded. I’ve meant to learn about them for a long time, but since (1) potentially long running lua, no matter whether its on 1 “lua thread” or 1000 causes Hammerspoon to block, but with #2304 that might no longer be an issue, and (2) I personally can code relatively quickly in Objective-C and can use a background thread it if I want.

If a true lua developer wanted to do something, they’d expect coroutines to exist and wouldn’t bother without them.

As I have come to understand the problem, the fix outlined as the first option above won’t be difficult, just tedious.

I’m opposed to burying our head in the sand by removing that the library – we won’t be using Lua, then, but a subset of it. Subsets are fine in memory constrained environments, not on a modern desktop.

As a first step, I think adding the following after the NSThread check in the sharedWithDelegate: method of LuaSkin will at least stop the application crashes, though 90% of our module functions/methods still won’t be usable within a coroutine:

    int isMainThread = lua_pushthread(self.L) ;
    lua_pop(self.L, 1) ; // remove thread we just pushed onto the stack
    if (!isMainThread) {
        luaL_error(self.L, "LuaSkin shared object incompatible with coroutines") ;
        return nil ;
    }

I’ll test this in a few hours to be certain.

Hello,

Coroutines are a fundamental control flow construct that ships with the language. Removing them is comparable to removing the function keyword. I only recently learned that Hammerspoon exists, and I am willing to try to fix the mess that currently prevents it from working with coroutines and with moonjit.

Ok, after some more pondering…

lua_lock and lua_unlock actually shouldn’t be necessary, if we stay on a single application thread…

We’ll still need to make LuaSkin multi lua_State* compatible for coroutine support (which is also a necessary step for supporting multiple application threads), but since only a single “lua thread” will ever be active at a given instant in a singe application threaded model, lua_lock/unlock don’t help us.

@cmsj, some ideas I’d like your comments on about how to make LuaSkin support different lua_State values (these both assume adding sharedWithState:(lua_State *L) and changing functions in our modules – but not in delegates or other callbacks – to use it instead of the shared contructor):

1. add (readonly?) property to store initial lua_State into during creation; when shared called, set self._L to this value; when sharedWithState:L called, set self._L to the L passed in. Pros: simple, Cons: may have to call sharedWithState:L multiple times within a function if it’s possible the current value of skin._L might have changed – currently (I think) this only affects the proposed hs_yield, but maybe also others, if we ever write C functions which utilize/participate in coroutines; won’t be sufficient if we ever add true multi application thread support.

2. create (and store to avoid re-creating?) new LuaSkin instance for each new lua_State seen. Pros: muti application thread safe (if/when we decide to go there), as long as no single lua_State is allowed to run on more than one thread at a time; Cons: more changes to LuaSkin because registered type helpers, etc. have to be moved to class properties; how do we detect when lua_thread is garbage collected (so we can remove its LuaSkin instance, if we saved it)? How do we handle a reload if multiple instances still exist (if we can attach a __gc to a lua_thread type, this might be a non-issue, but I don’t think we can)

Other possibilities I’m missing?

Re multiple application threads – If we do end up adding hs.yield, and maybe even consider attaching it to a debug hook so it runs after every n lua calls, the extra effort to support multiple application threads becomes less attractive to me. IMHO the times when yielding isn’t sufficient might be better served by moving something into Objective-C itself which is a lot faster and most things become easy (well, easier) to multithread and trigger a callback upon completion.

Just my (current) 2 cents… ask me again in a month and I’ll probably think something totally different 😜

Obviously few/none of our users currently utilize coroutines, or we’d have heard something by now, but it is a feature lua promotes, so its only a matter of time…

Possible short term band-aids that I can think of:

• remove coroutine library from our implementation of lua (either by modifying lua source, or adding coroutine = nil to hs._coresetup. Cowardly cop-out, and removes the possibility of the one case that does work – using them within a single file/function/block with pure lua.

• partially implement above fix – modify functions to use [LuaSkin sharedWithState:L] and throw a lua error if L doesn’t == skin.L that says something like LuaSkin incompatible with coroutines. Pro: should prevent crashes; Con: inconsistent module support within coroutines – older modules (pre LuaSkin) may partially work, if they don’t use LuaSkin to check arguments or manipulate data; newer or updated stuff won’t work at all, since we’ve pretty much standardized on some common patterns that use LuaSkin a lot.

• continue to hope no one notices. Cons: obvious.

In any case, if we do end up adding #2304, hs_yield can’t be invoked within a coroutine because we haven’t implemented lua_lock and lua_unlock (yet); but I think I can easily make this detect such a situation by checking lua_isyieldable() so this issue isn’t a show-stopper (though it does remove the need for one of the tests I was considering)

More thoughts if I think of them…

Ok, not quite “don’t work at all”…

if the code which creates and uses the coroutine is in one block, and doesn’t use anything which involves LuaSkin (i.e. it’s pure lua, not even a single function we’ve added that uses checkArgs:), then it will work:

This works (when pasted into the console as one “line” of input):

a = ""
cothread2 = coroutine.create(function()
    for n = 1, 5 do
        a = a .. "honk "..n.."\n"
        coroutine.yield()
    end
end)
while coroutine.status(cothread2) ~= "dead" do coroutine.resume(cothread2) end
print(a)

This crashes (when pasted into the console as one “line” of input) – yeah, it’s a stupid example, but hs.canCheckForUpdates was the first thing I found that only uses LuaSkin for a single [skin checkArgs:LS_TBREAK];, about the simplest usage I could think of):

a = ""
cothread2 = coroutine.create(function()
    for n = 1, 5 do
        a = a .. tostring(hs.canCheckForUpdates()) .. "\n"
        coroutine.yield()
    end
end)
while coroutine.status(cothread2) ~= "dead" do coroutine.resume(cothread2) end
print(a)

edit: made it more clear that blocks should be pasted in as one entry if testing in the console