Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

avoid creating nil callback for JobSpawn #3554

Merged
merged 1 commit into from
Dec 2, 2024

Conversation

matthias314
Copy link
Contributor

If a program started with shell.JobSpawn terminates and no callback is set, then micro crashes. For example, if I call the Lua function

function jobspawn(bp)
    shell.JobSpawn("xclock", {}, nil, nil, nil)
end

and close the xclock window, then I get ((on Ubuntu, with master) :

Micro encountered an error: runtime.errorString runtime error: invalid memory address or nil pointer dereference
runtime/panic.go:220 (0x44af36)
runtime/panic.go:219 (0x44af06)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:423 (0x8de2b5)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:400 (0x8dde97)
runtime/proc.go:250 (0x4373d2)
runtime/asm_amd64.s:1571 (0x465f21)
 
If you can reproduce this error, please report it at https://github.com/zyedidia/micro/issues

This PR fixes this bug.

f.Function(f.Output, f.Args)
if f.Function != nil {
f.Function(f.Output, f.Args)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't we rather prevent creating a dummy "job" in the first place? I.e.:

diff --git a/internal/shell/job.go b/internal/shell/job.go
index 6e1f4b18..766b9516 100644
--- a/internal/shell/job.go
+++ b/internal/shell/job.go
@@ -78,8 +78,10 @@ func JobSpawn(cmdName string, cmdArgs []string, onStdout, onStderr, onExit func(
 	go func() {
 		// Run the process in the background and create the onExit callback
 		proc.Run()
-		jobFunc := JobFunction{onExit, outbuf.String(), userargs}
-		Jobs <- jobFunc
+		if onExit != nil {
+			jobFunc := JobFunction{onExit, outbuf.String(), userargs}
+			Jobs <- jobFunc
+		}
 	}()
 
 	return &Job{proc, stdin}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to make it impossible to call JobSpawn without onExit callback? I think there are cases where you have no callback. For example, I'm currently trying to write some LaTeX support for micro. One functionality is to start a PDF viewer to see the generated PDF file. I start it with JobSpawn and no callback.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to make it impossible to call JobSpawn without onExit callback?

No, I don't. Take a closer look at the above code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for not being an expert in your code. So Run would start the program and Jobs is just there to handle the result. In that case your suggestion makes sense. Shall I update the PR?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, the Jobs channel is there to let DoEvent() in the main goroutine execute the onExit callback. So if there is no onExit callback, no point in sending anything to the Jobs channel in the first place.

Shall I update the PR?

Yes, if it works for you.

@matthias314
Copy link
Contributor Author

PR updated according to @dmaluka's suggestion

@matthias314 matthias314 changed the title bugfix: check for nil before executing callback for JobSpawn avoid creating nil callback for JobSpawn Dec 1, 2024
@JoeKar JoeKar merged commit 831e31d into zyedidia:master Dec 2, 2024
6 checks passed
@matthias314 matthias314 deleted the m3/jobspawn branch December 7, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants