-
Notifications
You must be signed in to change notification settings - Fork 105
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
Node process is not killed #16
Comments
Very likely. @clausreinke I guess that was the reason you used the awkward way of terminating the process? Did you find any background on why |
See ternjs/tern#97. There were various mismatches between "API available on windows" and "works on windows", not to mention "works the same way on windows". Convenience libraries for working with win32 don't seem to be installed by default. Then there is the intermediate shell process. As I'm no python expert, I worked around by using |
I resolved with the following code:
With I suppose there is not need now about If you like I can provide a PR. |
Avoiding the ternjs/tern@784b801#commitcomment-2846281 Your variant also isn't documented for python v2.7.1, does seem to appear in the subprocess source, but with some constants in |
Yes, the Can you try simply restoring the |
Since issues are easier to search, here are two related python bugs: Looks like |
Sorry misbutton... +1 to restore |
The bug reports suggest some kind of system behind those changes, and the python 3 docs still list So I played around a little and found that wrapping selective import in exception handling appears to work
with python 3.3.1 ( |
@clausreinke do you mean 2.7.4, you don't? |
@albertosantini 2.7.1 |
2.7.4 has neither |
Oh, wait, of course it doesn't. I'm not on Windows. |
@clausreinke Do you have time to submit a patch that works for you? |
Having tried this, there are some disadvantages:
|
On Unix, Tern handles SIGTERM and does clean up. But I'm not sure how that maps to windows (both on the node.js and Python side). I'm okay with just reverting to running |
In windows we have two options: taskkill or Popen with startupinfo. For shell=True see the warning under Frequently Used Arguments for details: startupinfo for windows is supported in 2.7.5 and in 3.3.2 (see Popen Helpers section). Googling about to kill a tree of processes (for instance, cmd + tern process), it seems the simplest way is taskkill on windows. |
Please see patch 16493d3 . Wholly untested, but might just be trivial enough to still work. |
For me it is ok (win 7 64bit / python 2.7.5 / vim 7.3.46). I tested the patch without a tern project file and with For every javascript file , I have a node process. When I quit Vim, all node processes are terminated. |
As far as I can tell, TerminateProcess is the most brutal and uncooperative way to end a process, the final step. Killing a process from the Taskmanager tries something more cooperative first, but not POSIX style. Neither python nor nodejs seem to participate in the cooperative protocols(?), so it is just a "hammer over the head", no clean up. Again, a |
Merging in dupe #28 here #fixme |
I grew tired of tern leaving
That seems to allow tern to shut down properly while clearing away |
Fair enough. I guess the windows situation is crappy enough to warrant such a workaround. Will you submit a pull request? |
I found an even easier way (soft exit tern when its stdin ends). Pull requests: ternjs/tern#242 and #40 |
@clausreinke's improvement has been merged and appears to work on both Linux and Windows (and thus most likely also OS X). Closing. |
I am using tern for vim and I have the merged change: ack 'stdin=subprocess.PIPE, stdout=subprocess.PIPE' however I have over 100 node procs open right now. This is definitely not fixed. Please reopen this issue. |
@delvarworld are you using the matching |
I just tried installing the latest version (cloned this repo into bundle, ran npm install inside of it) Right now I have 9 node procs open. Not sure if it's rising, will report back. But vim is already slowing down. |
11 node procs open me@~/.vim/bundle/tern_for_vim (master) $ npm list Ugh. Does anyone actually use tern for vim successfully? Are they on OSX? |
Definitely still happening. Please reopen this ticket. |
@delvarworld can't really help you, since this doesn't happen for me and I don't have a Mac. Since closing a process on end of On the other hand, that increases your chances of finding the root cause yourself: just start with a small test case, then build up complexity until you either have a working plugin or a small test case that does not work as expected. Here is a minimal file to test the behavior the patch in question is making use of:
The process prints timings until its stdin ends (piping in a finite input, or typing If that works as expected, add a python script that starts this process via |
This is actually biting me when trying to run tern dockerized - I think Kubernetes ends stdin to the container, which causes tern to kill itself. I'm not sure if this should be a concern, but it took me quite a long time to debug. |
re: my comment ternjs/tern_for_vim#16 (comment), I think this would have greatly improved my ability to figure out what was going wrong with trying to run tern.
re: my comment ternjs/tern_for_vim#16 (comment), I think this would have greatly improved my ability to figure out what was going wrong with trying to run tern.
re: my comment ternjs/tern_for_vim#16 (comment), I think this would have greatly improved my ability to figure out what was going wrong with trying to run tern.
Node process is not terminated when Vim quits (win7).
I think it is due to this change:
c48a908#L0L83
The text was updated successfully, but these errors were encountered: