-
-
Notifications
You must be signed in to change notification settings - Fork 303
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
Fix process hangs when daemon starts #373
Conversation
daemon/daemon.go
Outdated
return errors.New("cannot find elvish: " + err.Error()) | ||
} | ||
binPath = result | ||
if !filepath.IsAbs(binPath) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that it is better to use the homegrown searching utility (util.Search
) in place of exec.LookPath
in consistency with the rest of Elvish.
daemon/daemon.go
Outdated
binPath = result | ||
if !filepath.IsAbs(binPath) { | ||
var err error | ||
binPath, err = exec.LookPath(os.Args[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original code has the upside of not crashing when size of os.Args
is 0 :).
Also, when Elvish is used as a login shell, os.Args[0]
contains -elvish
, and this code will search -elvish
instead of elvish
.
I think the original code is fine here...
daemon/daemon.go
Outdated
"-sock", d.SockPath, | ||
"-logprefix", d.LogPathPrefix, | ||
) | ||
return cmd.Run() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to understand what is happening under the scene than using a different interface to do the job :) According to the doc cmd.Run
waits for the subprocess to terminate, that is probably some step we should add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to understand what is happening under the scene than using a different interface to do the job :)
Agreed. The point here is, trying not to re-invent the wheel by using high level API when possible since it is not recommended to use "syscall" package directly according to its doc.
A quick gdb showed the daemon was stuck during the syscall, which remind me to check the flags done internally by exec.Command
.
(gdb) bt
#0 syscall.Syscall () at /usr/lib/go-1.8/src/syscall/asm_linux_amd64.s:27
#1 0x000000000047e516 in syscall.readlen (fd=1, p=0xc420125ae0 "", np=8, n=13, err=...) at /usr/lib/go-1.8/src/syscall/zsyscall_linux_amd64.go:1085
#2 0x0000000000478313 in syscall.forkExec (argv0=..., argv=..., attr=0xc420125da0, pid=10, err=...) at /usr/lib/go-1.8/src/syscall/exec_unix.go:202
#3 0x0000000000478754 in syscall.ForkExec (argv0=..., argv=..., attr=0xc420125da0, pid=140729429880769, err=...)
at /usr/lib/go-1.8/src/syscall/exec_unix.go:235
#4 0x0000000000541bfa in github.com/elves/elvish/daemon.forkExec (attr=0xc420125da0, forkLevel=1, binPath=..., dbPath=..., sockPath=..., logPathPrefix=...,
~r6=...) at /home/go/src/github.com/elves/elvish/daemon/daemon.go:120
#5 0x0000000000541a78 in github.com/elves/elvish/daemon.(*Daemon).pseudoFork (d=0xc4201145f0, attr=0xc420125da0, ~r1=842351593000)
at /home/go/src/github.com/elves/elvish/.build/src/github.com/elves/elvish/daemon/daemon.go:104
#6 0x000000000054173a in github.com/elves/elvish/daemon.(*Daemon).Main (d=0xc4201145f0, serve={void (struct string, struct string)} 0xc420125e48,
~r1=14689824) at /home/go/src/github.com/elves/elvish/daemon/daemon.go:67
#7 0x0000000000816869 in main.main () at /home/go/src/github.com/elves/elvish/main.go:121
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably best to use the os/exec
interface to implement forkExec
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deal. Using os.StartProcess
instead to fully close stdin/stdout/stderr.
dd325b7
to
7f5365b
Compare
daemon/daemon.go
Outdated
// pseudoFork forks a daemon. It is supposed to be called from the daemon. | ||
func (d *Daemon) pseudoFork(attr *syscall.ProcAttr) int { | ||
err := forkExec(attr, d.Forked+1, d.BinPath, d.DbPath, d.SockPath, d.LogPathPrefix) | ||
p, err := os.StartProcess(binPath, []string{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is repeated below; please make it a function.
The parent hangs on syscall When creating daemon from client. Instead, use highlevel interface to bring daemon up.
The parent hangs on syscall When creating daemon from client. Instead,
use highlevel interface to bring daemon up.