I do not know what use cases does RunBackgroundShell() have since it's somewhat similar with RunCommand(), but JobStart() can be used like this to mostly achieve the purpose of the function added:

local micro = import("micro")
local shell = import("micro/shell")

local job

local function onexit(output, data)
    local state = job.ProcessState
    -- ProcessState is set if process (sh if using JobStart) exits
    -- onExit is called even if an error occurs with something like I/O within
    -- Go, but it is probably rare
    --if state == nil then return end

    micro.TermMessage(state:Success() and output or state:String())
end

function init()
    job = shell.JobStart("kill $$", nil, nil, onexit)
end

I think it is better to instead change the function so that it returns an error when occuring on process startup, and passes the error that cmd.Wait returns to onExit.

The main thing I wanted was to able to do something like RunCommand() but doesn't block the main thread, for example something like a curl command which might take a couple seconds.

Yeah it's true that I can do JobStart() (Although only Linux since it uses sh -c which this PR also fix) and I actually didn't know you can get the exit state with job.ProcessState.

But even then you still need to keep track of the command execution by storing it in global (probably in a map or some sort?)

If this PR gets merged, what you need to do instead it's just

local function onexit(output, err)
    -- Handle err here...
    micro.TermMessage(output)
end

function init()
    local retFunc, err = shell.RunBackgroundShell("curl whatever")
    -- Handle err...
    shell.LaunchBackgroundShellFunc(retFunc, onexit)
end

where you can handle the error directly in the callback as param, not to mention it has a simpler interface than JobStart(...)

In my mind, the Job* functions make sense for long-running tasks like LSP but overkill and painful to manage for a command that takes a few seconds to complete.

But even then you still need to keep track of the command execution by storing it in global (probably in a map or some sort?)

Yes, that is right. Tables in userargs are copied since they are converted to an array or map in Go, so passing a table containing the job like this cannot be done:

local t = { i = 1 }
-- in onexit, userargs[1] is { i = 1 } with job not set
t.job = shell.JobStart("sleep 1", nil, nil, onexit, t)
t.i = 2

There are workarounds, so I don't think changing the function to pass Lua values without conversion needs to be prioritized. Arrays converted using gopher-luar can be iterated by writing array() unlike ipairs(t) used with tables, so changing this has a low chance to break existing plugins.

where you can handle the error directly in the callback as param, not to mention it has a simpler interface than JobStart(...)

I agree that the interface is simpler, especially since no objects have to be created in plugins to check errors. However, the interface seems a bit odd since the names are similar even though RunBackgroundShell and the function returned doesn't run programs asynchronously.

A function that's like a simplified version of JobStart, takes command as argument and immediately runs it asynchronously, while it returns errors or passes them to the callback may be better.

In my mind, the Job* functions make sense for long-running tasks like LSP but overkill and painful to manage for a command that takes a few seconds to complete.

I have no experience with adding APIs so I'll leave this to maintainers or others to decide and discuss.

But even then you still need to keep track of the command execution by storing it in global (probably in a map or some sort?)

Yes, that is right. Tables in userargs are copied since they are converted to an array or map in Go, so passing a table containing the job like this cannot be done:

local t = { i = 1 }
-- in onexit, userargs[1] is { i = 1 } with job not set
t.job = shell.JobStart("sleep 1", nil, nil, onexit, t)
t.i = 2

I think you misunderstood what I meant. I meant you need to keep track of the job object returned by the JobStart(...) function for each call, if you need to know the return code / result of that command that is.

where you can handle the error directly in the callback as param, not to mention it has a simpler interface than JobStart(...)

I agree that the interface is simpler, especially since no objects have to be created in plugins to check errors. However, the interface seems a bit odd since the names are similar even though RunBackgroundShell and the function returned doesn't run programs asynchronously.

Yes, I totally agree that RunBackgroundShell doesn't actually run in the background is very confusing 😂

A function that's like a simplified version of JobStart, takes command as argument and immediately runs it asynchronously, while it returns errors or passes them to the callback may be better.

Yeah I would like to do that but I feel like that will be confusing as well since that will be doing what RunBackgroundShell is supposed to do.

Honestly though, I am happy to modify RunBackgroundShell to just run the command in the background if the maintainers are happy to break backward compatibility.

But I think at the minimum, we should at least merge the fix for JobStart() so that it works on Windows as well.

I think you misunderstood what I meant. I meant you need to keep track of the job object returned by the JobStart(...) function for each call, if you need to know the return code / result of that command that is.

Sorry, I meant to mention another approach on keeping track of the job object, that currently doesn't work even though it would be better than using something like a map in some cases.

But I think at the minimum, we should at least merge the fix for JobStart() so that it works on Windows as well.

I think it's better to submit this change in another pull request even if it's a lot smaller. I have done something similar in #3672 but I honestly have other pull requests where I haven't done such yet.

Regarding problems with JobStart() on Windows, you could use JobSpawn() instead. (Although I don't disagree that making JobStart() work on Windows is a good idea.)

Regarding this:

But even then you still need to keep track of the command execution by storing it in global (probably in a map or some sort?)

You could use a closure:

local function onexit(output, job)
    local state = job.ProcessState
    micro.TermMessage(state:Success() and output or state:String())
end

function init()
    local job
    job = shell.JobSpawn("date", {}, nil, nil, function(output)
        onexit(output, job)
    end)
end

It works, but it is actually incorrect, because it is racy: the background command may have already finished (and thus onexit may already execute) before JobSpawn() has returned, i.e. when job is still nil. Looks like I'm actually hitting this race, when running a non-existing command (e.g. if I replace "date" with "date1"), causing micro crashing with Lua error attempt to index a non-table object(nil) with key 'Success'.

@niten94 's example in #3692 (comment) has the same problem, I suppose.

So it appears that Job*() functions don't provide a safe way to get the exit status of the background command. Perhaps would could add e.g. JobCreate() which would create and return a job without starting it, and another function to start this returned job, when we could safely pass this returned job to this other function's onExit callback... But then perhaps we'd be better off just exporting Go's https://pkg.go.dev/os/exec interfaces to Lua (like we already export https://pkg.go.dev/os and a bunch of other Go packages).

...I agree that the RunBackgroundShell() interface seems pointless. Instead of adding this LaunchBackgroundShellFunc() to adjust to the existing stupidity, shouldn't we just add a brand new interface e.g. RunBackgroundCommand() which just does the right thing? (while keeping the existing RunBackgroundShell() as is, just in case someone is actually using it.)

@dmaluka @niten94
Okay, then I will just modify LaunchBackgroundShellFunc() to RunBackgroundCommand() which will be similar to existing RunCommand() and might as well modify the documentation to deprecate RunBackgroundShell().

@Neko-Box-Coder:

Okay, then I will just modify LaunchBackgroundShellFunc() to RunBackgroundCommand() which will be similar to existing RunCommand() and might as well modify the documentation to deprecate RunBackgroundShell().

Alright, I think that would be good.

@dmaluka:

It works, but it is actually incorrect, because it is racy: the background command may have already finished (and thus onexit may already execute) before JobSpawn() has returned, i.e. when job is still nil. Looks like I'm actually hitting this race, when running a non-existing command (e.g. if I replace "date" with "date1"), causing micro crashing with Lua error attempt to index a non-table object(nil) with key 'Success'.

I don't see how onexit can execute before JobSpawn returns, since the callback is executed in the main event loop. I think you may have looked at old documentation, since it was changed in Go 1.20 to mention that ProcessState is set if the process successfully starts.

Perhaps would could add e.g. JobCreate() which would create and return a job without starting it, and another function to start this returned job, when we could safely pass this returned job to this other function's onExit callback... But then perhaps we'd be better off just exporting Go's https://pkg.go.dev/os/exec interfaces to Lua

Plugins can also set PWD and variables after calling JobCreate, so maybe we should implement this.

I've came up with naming the latter function JobLaunch, adding a method named Launch on the job struct, or having JobCreate include a start function in the return values. I personally don't like JobLaunch, since 3 job functions with similar verbs in names seems too much.

I don't see any advantages in exporting os/exec, since receiving an error and modifying Cmd struct can be done with the suggested functions.

I don't see how onexit can execute before JobSpawn returns, since the callback is executed in the main event loop.

Ah, indeed... So, it is not job but job.ProcessState that is still nil.

So, a corrected version:

local function onexit(output, job)
    local state = job.ProcessState
    if state ~= nil then
        micro.TermMessage(state:Success() and output or state:String())
    else
        micro.TermMessage("failed to execute date")
    end
end

function init()
    local job
    job = shell.JobSpawn("date", {}, nil, nil, function(output)
        onexit(output, job)
    end)
end

It works reliably, but it doesn't allow to report the exact reason why it failed to execute...