Adding lua function to use result of RunBackgroundShell #3692

Neko-Box-Coder · 2025-03-12T23:57:44Z

Currently, RunBackgroundShell() returns a function and an error. I don't think there's a way to run that function asynchronously from lua plugin.

Adding a helper lua function for that.

Although there's an additional error to the return of the returned function from RunBackgroundShell(), this has no impact the lua plugins that are currently using it since it first return var is the same. I have tested this with a lua plugin.

niten94 · 2025-03-26T14:54:49Z

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.

Neko-Box-Coder · 2025-03-26T20:08:11Z

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.

niten94 · 2025-03-28T23:30:37Z

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.

Neko-Box-Coder · 2025-03-29T01:17:42Z

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.

niten94 · 2025-03-30T13:20:02Z

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.

dmaluka · 2025-04-26T16:36:36Z

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.)

Neko-Box-Coder · 2025-04-26T17:20:19Z

@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().

niten94 · 2025-05-03T13:24:22Z

@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.

dmaluka · 2025-05-04T14:27:26Z

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...

niten94 · 2025-09-20T14:07:26Z

internal/action/command.go

+			output = fmt.Sprint(args[0], " exited with error: ", runErr)
+		}
+		InfoBar.Message(output)
+		screen.Redraw()


The screen doesn't need to be redrawn here now, since the event loop does it after the callback is ran.

niten94 · 2025-09-20T14:23:23Z

internal/shell/shell.go

+				errVal, ok := args[0].(error)
+				if ok {
+					cb(output, errVal)
+				} else {
+					cb(output, nil)
+				}


I think it's fine to directly run the callback without checking ok, and assign the boolean result to _ instead.

True. Updated.

niten94 · 2025-09-20T14:38:33Z

internal/shell/shell.go

+// RunBackgroundCommand runs a shell command in the background by accepting
+// an input for running the command and an optional callback function for


To match the style of other comments in Micro and be slightly clearer about behavior, it may be better to write the description as something like this: "RunBackgroundCommand runs a shell command and an optional callback on exit..."

Yep, I have shortened the comment.

niten94 · 2025-09-20T14:53:33Z

runtime/help/plugins.md

+    - `RunBackgroundCommand(input string, cb func(string, error)) error`:
+       runs a shell command in the background by accepting an input for
+       running the command and an optional callback function for the 
+       command output or error if any.


It could be written instead that the function is the same as ExecBackgroundCommand() and parses the string argument like RunCommand(), to avoid adding similar text while referencing a slightly detailed explanation on the parsing.

RunBackgroundCommand is different since it returns an error. I updated the docs to point to the non-background equivalents.

niten94 · 2025-09-20T15:08:07Z

internal/shell/shell.go

+	go func() {
+		output, runErr := ExecCommand(name, args[0:]...)
+		if cb != nil {
+			wrapperFunc := func(output string, args []interface{}) {


interface{} was replaced with any in all parts of Micro's code in #3834, so it should be done with this PR as well.

Yep, updated to use any now.

niten94 · 2025-09-20T15:24:25Z

I'm sorry for the slightly confusing comments I left before and not leaving any review. I've slightly used and read about cmd.exe sometimes before, and I also think the shell invocation for JobStart() is fine.

niten94

Only documentation suggestions at this point, but this PR should be good.

I should've mentioned this a lot earlier but I honestly avoided reviewing this PR, because I had been thinking a new shell API could replace most of the existing and added functions.

After thinking more, I think a new plugin API should be introduced as a whole instead. I plan to create an issue for it sometime and the old API would only always remain deprecated. Still, it's unlikely a large new API can be introduced around this time.

niten94 · 2025-10-28T18:25:33Z

runtime/help/plugins.md

+    - `ExecBackgroundCommand(cb func(string, error), name string, args ...string)`:
+       similar to ExecCommand except it runs in the background and accept


This applies to the plugin documentation of RunBackgroundCommand() as well, but it's better to enclose "ExecCommand" in backticks because this is markdown text. However, identifiers wouldn't be quoted in Go comments.

niten94 · 2025-10-28T18:36:11Z

internal/shell/shell.go

+// ExecBackgroundCommand is similar to ExecCommand except it runs in the
+// background and accept an optional callback function for the command output
+// or error if any.


This applies to the description in plugin.md as well, but comma could be added before "except" and "accept" should be replaced with "accepts".

~~Also, I think the comment will be slightly clearer and consistent if "if any." (including period) is removed.~~

Edit: Instead of my suggestion in the previous sentence, replacing the last line with "or an error if any" (and still removing the period) might be better.

niten94 · 2025-10-28T18:40:49Z

runtime/help/plugins.md

+    - `RunBackgroundCommand(input string, cb func(string, error)) error`:
+       similar to RunCommand except it runs in the background and accept
+       an optional callback function for the command output or error if any.
+       It returns an error immediately if it fails to split the input command


Period should probably be added here at the end like other function descriptions in plugin documentation.

niten94 · 2025-10-28T18:49:26Z

internal/shell/shell.go

+// RunBackgroundCommand is similar to RunCommand except it runs in the 
+// background and accept an optional callback function for the command output
+// or error if any.
+// It returns an error immediately if it fails to split the input command


Similar with the suggestion of the comment on ExecBackgroundCommand(). It may also slightly look better to replace "It returns" with "Returns" in the comment only.

Using Jobs channel to call lua callback

niten94

LGTM. I apologize that my previous comment is a bit incosiderate, including the idea of deprecating APIs which isn't a good idea too.

Neko-Box-Coder · 2025-11-24T10:04:17Z

No worries. I was just a bit busy recently and hence the late edit.
I am fine with a new shell API if that improves things, but we still probably need to maintain the current API for now, even if we decide to deprecate things.

Neko-Box-Coder changed the title ~~Adding lua function to use result of updated RunBackgroundShell~~ Adding lua function to use result of RunBackgroundShell Mar 12, 2025

Neko-Box-Coder force-pushed the FixRunBackgroundShell branch 2 times, most recently from 03031e7 to 4658fa2 Compare May 6, 2025 17:52

Neko-Box-Coder force-pushed the FixRunBackgroundShell branch from 4658fa2 to c626395 Compare September 6, 2025 17:19

niten94 reviewed Sep 20, 2025

View reviewed changes

Neko-Box-Coder force-pushed the FixRunBackgroundShell branch 3 times, most recently from 1bfb8e3 to f870350 Compare October 11, 2025 20:40

niten94 reviewed Oct 28, 2025

View reviewed changes

Neko-Box-Coder added 2 commits November 18, 2025 19:22

Adding lua function to use result of updated RunBackgroundShell

1e9205a

Using Jobs channel to call lua callback

Adding windows support for JobStart()

2209957

Neko-Box-Coder force-pushed the FixRunBackgroundShell branch from f870350 to 2209957 Compare November 18, 2025 19:22

niten94 approved these changes Nov 23, 2025

View reviewed changes

		// RunBackgroundCommand runs a shell command in the background by accepting
		// an input for running the command and an optional callback function for

		- `ExecBackgroundCommand(cb func(string, error), name string, args ...string)`:
		similar to ExecCommand except it runs in the background and accept

Adding lua function to use result of RunBackgroundShell #3692

Are you sure you want to change the base?

Adding lua function to use result of RunBackgroundShell #3692

Uh oh!

Conversation

Neko-Box-Coder commented Mar 12, 2025

Uh oh!

niten94 commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Neko-Box-Coder commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niten94 commented Mar 28, 2025

Uh oh!

Neko-Box-Coder commented Mar 29, 2025

Uh oh!

niten94 commented Mar 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmaluka commented Apr 26, 2025

Uh oh!

Neko-Box-Coder commented Apr 26, 2025

Uh oh!

niten94 commented May 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmaluka commented May 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neko-Box-Coder Oct 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niten94 commented Sep 20, 2025

Uh oh!

niten94 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niten94 Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niten94 left a comment

Choose a reason for hiding this comment

Uh oh!

Neko-Box-Coder commented Nov 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

niten94 commented Mar 26, 2025 •

edited

Loading

Neko-Box-Coder commented Mar 26, 2025 •

edited

Loading

niten94 commented Mar 30, 2025 •

edited

Loading

niten94 commented May 3, 2025 •

edited

Loading

Neko-Box-Coder Oct 11, 2025 •

edited

Loading

niten94 left a comment •

edited

Loading

niten94 Oct 28, 2025 •

edited

Loading