fix start #7

Merged
merged 1 commit into from Sep 19, 2012

3 participants

@guybrush

I have the same problem as mentioned in #2 and #3

Basically i dont know if this PR is ok, or if @jgallen23 's PR is better - anyways this works for me :)

@tj
Owner
tj commented Sep 19, 2012

seems odd to me, most programs use /bin/sh, hell even node uses /bin/sh haha so can you not exec() with node?

@guybrush

yes i can exec() with node

wtf :p

i have to read some manuals

@guybrush

im btw on debian where /bin/sh links to /bin/dash

@tj
Owner
tj commented Sep 19, 2012

maybe try system(cmd) instead of the exec

@guybrush

nah its the same with system(cmd) the node-process stays alive

@tj
Owner
tj commented Sep 19, 2012

interesting, dash -c cmd must be forking or something, bash exec()s and replaces itself so the signals work fine, if you look at it in htop does it still show dash?

@guybrush

sorry i dont understand

http://i.imgur.com/fEf21.png (this is with execl('/bin/sh','sh',...)

i am still reading execl(3) :D

@tj
Owner
tj commented Sep 19, 2012

the exec* syscalls effectively replace what invoked it, unless it errors it never returns because the caller has been replaced. for ex here's my mon with bash:

@guybrush

oooh i see

@tj
Owner
tj commented Sep 19, 2012

I think that's the problem we're seeing here with the different shells

@guybrush

really i understand that its cool to have lightweight shell for faster boot and stuff and linux switching from bash to dash... but platform-specific scripting just sucks!

@guybrush

so for me... i will go with @jgallen23 's PR, since i am on linux and all my machines and stuff are linux with debian

this PR (#3) is ok, right? i mean it doesn't break soemthing or is wrong in some weird way i dont understand

like my PR (#7) is just wrong, after reading exec*(3) :D

@tj
Owner
tj commented Sep 19, 2012

yeah it works fine for me, but you'll still have to signal that child proc i guess

@tj
Owner
tj commented Sep 19, 2012

im still curious how node works and this does not

@tj tj merged commit 0697476 into tj:master Sep 19, 2012
@guybrush

awesome, big thx for spending some time to looking into it :)

@jgallen23

@guybrush I've been using my fork on ubuntu for awhile now, no problems so far

@tj
Owner
tj commented Sep 19, 2012

np, if you ever get bored strace node and take a look at the exec calls haha this seems super weird

@guybrush

@visionmedia unroll please the commit! this does not work, my PR is wrong - i meant to use @jgallen23 's PR (sorry for the noise)

and i am not sure if execl("/bin/bash", "sh", "-c", cmd, 0); works for other platforms but linux with debian (i.e. /bin/bash -thing)

also i want to repeat, platform-specific scripting sucks...

@guybrush

or maybe just merge nothing - since its really platform-dependent?

since i am curious i will look into node how they do it :p

@tj
Owner
tj commented Sep 19, 2012

im really surprised if system() doesn't work, that's the whole point of that function

@guybrush

system() starts the child properly but sending sigkill/sigquit to the mon does not kill the child

on my setup with execl("/bin/bash", "sh", "-c", cmd, 0); i can send sigkill/sigquit to the mon-proc and it kills the child properly

@tj
Owner
tj commented Sep 19, 2012

yeah that's fine, that's expected, you have to signal the child, I'm not relaying anything, and you can't trap SIGKILL so when you kill the parent init adopts the child

@guybrush

ahhhh man im sorry, i meant killing the the child.

I start with mon mon -d "node test" -p file.pid and stop it with kill -s SIGKILL $(cat file.pid). so mon will restart the node test which will not work, since test.js runs a http-server on a port and will report EADDRINUSE

with system(cmd)

with execl("/bin/bash", "sh", "-c", cmd, 0)

@tj
Owner
tj commented Sep 19, 2012

maybe system() was a bad suggestion, the other could would need some reworking since im using WEXITSTATUS etc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment