atom-community / atom-script

:runner: Run ( scripts | selections | source ) in Atom
https://atom.io/packages/script
MIT License
733 stars 269 forks source link

Terminating go script doesn't kill child processes #453

Open bsferrazza opened 9 years ago

bsferrazza commented 9 years ago

Using a basic golang script, such as the one below that serves a simple webpage, is not properly killed using the Mac OS X version of Atom and atom-script. Assuming the name of the script is 'hello.go', when the script is run using atom-script, you can see that both a 'go' and a 'hello' process are actively running. When the script is terminated using 'Ctrl-C', only the 'go' process is killed and not its children processes, such as 'hello'. This is problematic, as it requires a manual process kill of all children process before the script can be re-run. If the script is run from the command line using 'go run' it kills all processes when 'Ctrl-C' is issued. I'm curious how the process is being terminated? Through a standard SIGTERM?

package main

import (
    "net/http"
)

func handler(w http.ResponseWriter, r *http.Request) {
    w.Write([]byte("Hello World!"))
}

func main() {
    http.HandleFunc("/", handler)
    http.ListenAndServe(":8080", nil)
}
davewv31 commented 9 years ago

I've been running into the same problem. atom-script uses bufferedProcess provided by Atom.

From script-view.coffee

  stop: ->
    # Kill existing process if available
    if @bufferedProcess?
      @display 'stdout', '^C'
      @headerView.setStatus 'kill'
      @bufferedProcess.kill()
      @bufferedProcess = null

The problem is buffedProcess on not-windows atom doesn't seem to kill children of the process, which means that child processes spawned continue to run on. I'm creating an issue on atom, we'll see what happens.

atom/atom#7252

rgbkrk commented 9 years ago

Thanks for reporting it up on Atom. We've been using BufferedProcess from Atom itself instead of using child_process's spawn or exec, hoping it would be fairly standard across usage in Atom. We'd certainly welcome a PR migrating to the node-native way.