Plugin using `RunInteractiveShell` crashes Micro

[usfbih8u]

Description

Micro crashes in onBufferOpen(), onBufPaneOpen(), postinit(), preinit(), and init() if you attempt to run an interactive shell (RunInteractiveShell()) before signal setup is complete (cmd/micro/micro.go:447).

-- USE COMMAND: micro -filetype myuniquespecialfiletype
function postinit()
    local bp = micro.CurPane()
    if bp then
        local ft = bp.Buf.Settings["filetype"]
        if ft == "myuniquespecialfiletype" then
        -- This was a call to bp:HandleCommand() with my plugin,
        -- but the crash was actually caused by the shell.RunInteractiveShell() 
        -- call.
        -- bp:HandleCommand("myplugin")
        shell.RunInteractiveShell("fzf-for-example", true, true)              
        bp:QuitAll()
    end
end

Crash:

Plugin test: os/signal: Notify using nil channel
    stack traceback:
            [G]: in function 'HandleCommand'
            main:17: in main chunk
            [G]: ?     

I attempted to use AfterFunc() from the time package, but it triggered a different type of crash.

function postinit()
    local gotime = import("time")
    gotime.AfterFunc(4 * gotime.Second, function()
        local bp = micro.CurPane()
        if bp then
            local ft = bp.Buf.Settings["filetype"]
            if ft == "myuniquespecialfiletype" then
                shell.RunInteractiveShell("fzf-for-example", true, true)              
                bp:QuitAll()
            end
        end
    end)
end

Micro encountered an error: runtime.errorString runtime error: invalid memory address or nil pointer dereference
runtime/panic.go:262 (0x472b19)
runtime/panic.go:261 (0x472ae9)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:490 (0x94bb41)
github.com/zyedidia/micro/v2/cmd/micro/micro.go:481 (0x94ba50)
internal/runtime/atomic/types.go:194 (0x43bc6b)
runtime/asm_amd64.s:1700 (0x478de1)

Using After() from micro's LuaTable resolved the issue (this presumably waits until all setup is complete).

Environment

• Version: 2.0.15 release
• OS: Linux
• Terminal: all

[JoeKar]

Looks like we can/should move...

	screen.Events = make(chan tcell.Event)

	util.Sigterm = make(chan os.Signal, 1)
	sighup = make(chan os.Signal, 1)
	signal.Notify(util.Sigterm, syscall.SIGTERM, syscall.SIGINT, syscall.SIGQUIT, syscall.SIGABRT)
	signal.Notify(sighup, syscall.SIGHUP)

	timerChan = make(chan func())

...behind the successful initialization of the screen.

[dmaluka]

I've checked that @JoeKar 's #4027 fixes the 1st crash.

It doesn't fix the 2nd crash (the one with AfterFunc()) but that crash is not due to a bug in micro, it is due to the fact that using Go's AfterFunc() is not safe, since it runs the timer callback completely unsynchronized with the rest of micro. I.e. the timer callback fires at a random moment, when micro is in an unknown state, so it may not be ready to handle the callback. In this case, the callback fires when screen.Screen is temporarily nil (due to screen.TempFini() called from RunInteractiveShell()), so the event loop crashes on screen.Screen.Fill().

And yes, with micro's After() there is no such problem. Micro's After() is what you should use instead of AfterFunc().

BTW in #3023, when I added After(), I removed exposing AfterFunc() to lua, precisely for this reason (i.e. because it is unsafe to use). Then I brought it back in #3211, because people complained about breaking compatibility. And here we are.

BTW I've recalled that I brought it back but with such a TODO:

	// TODO: these raw Go timer APIs don't provide any synchronization
	// with micro. Stop exposing them to lua once plugins switch to using
	// the safer micro.After() interface instead. See issue #3209

And I see that at least @Andriamanitra 's plugin https://github.com/Andriamanitra/mlsp, due to which @Andriamanitra originally reported #3209, did switch to micro.After(). So it might be a good time to remove AfterFunc() once again (and then wait until someone else complains again)?

And BTW I've also found out that I also had such an idea in #3444 (comment):

...Now I'm actually thinking about removing this old timer API once again, but this time don't remove this singe AfterFunc function but make it an alias for After, rather than the raw Go's time.AfterFunc.

[JoeKar]

It doesn't fix the 2nd crash (the one with AfterFunc())

I don't know why I didn't respond to this, but maybe it was because of...

Using After() from micro's LuaTable resolved the issue (this presumably waits until all setup is complete).

...and I fully ignored it.

this time don't remove this singe AfterFunc function but make it an alias for After, rather than the raw Go's time.AfterFunc.

Yes, sounds like the way to go.

Edit:
We should handle this then in a separate PR.