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

No zombies #4

Merged
3 commits merged into from Jan 6, 2011

Conversation

Projects
None yet
2 participants
@guns
Copy link

guns commented Jan 5, 2011

Hi, I switched to your preview plugin from another markdown preview plugin, and I'm having a great time.

Attached is a patch that reaps zombies created by Kernel#fork and uses more standard approaches for shell escaping. The details are in the patch header.

Btw, if you do alot of shelling out from ruby, I suggest you take a look at the shellwords module and the new 1.9 features for system, exec, and spawn. Most of the time though, you can just feed shell params to system and friends as separate params (and not a single string) and be fine.

Cheers
Sung

@guns

This comment has been minimized.

Copy link
Author

guns commented Jan 5, 2011

Oops, patch should work properly now. I didn't realize Vim changes the values of $stdout and $stderr. In this case, redirecting the STDOUT, STDERR constants is the right thing to do.

@greyblake

This comment has been minimized.

Copy link
Owner

greyblake commented Jan 6, 2011

Hi, Sung! Let me thank you for your effort!
You are right that zombies should be handled appropriate way.
But is it work for you? In my case it doesn't help. A zombie still exists after closing a browser.I spent some time trying to understand why so.
As ruby doc says, Process.detach creates new thread and waits until forked process finishes.
But main thread finished earlier than browser is closed. So the thread what was created by Process.detach wouldn't handle a zombie, because it will be aborted when the main thread is closed

You can do a simple test in your Vim editor(type this, save a file and do :so %):

ruby <<RUBY_END
Thread.new do
  sleep 1
  system "echo SOMETHING > /tmp/vim_rb.log";
end
RUBY_END

In this case you'll never see tmp/vim_rb.log file.
And if you try this:
ruby <<RUBY_END
Thread.new do
sleep 1
system "echo SOMETHING > /tmp/vim_rb.log";
end
sleep 2
RUBY_END
/tmp/vim_rb.log will be created.

Please let me know, if I'm wrong or if your changes really work for you.
By the way my environment is:
OS kernel: Linux 2.6.32-5-amd64
Shell: Bash 4.1.5
Vim: 7.3 (with ruby 1.8.7 patch 302).

Thanks.
Sergey.

@guns

This comment has been minimized.

Copy link
Author

guns commented Jan 7, 2011

But main thread finished earlier than browser is closed. So the thread what was created by Process.detach wouldn't handle a zombie, because it will be aborted when the main thread is closed

Ah, it seems there's a bit I don't understand about Vim's ruby integration. You're right, zombies were still being created. This would not be happening if vim didn't kill the thread running the ruby interpreter, or if it ran ruby commands in a subprocess (where we could just tell it to wait for children to die with Process.wait). I will look into it further and try to get it right!

(I accidentally pushed a commit; please ignore)

@greyblake

This comment has been minimized.

Copy link
Owner

greyblake commented Jan 7, 2011

However, thanks:)

guns
Double fork to ensure reaping thread via Process#detach is successful
The last attempt against zombie creation was unsuccessful because the
reaping thread was aborted along with the main vim-ruby thread.

If we double fork the browser program we can:

 * ensure that Process#detach will correctly setup and reap the
   grandchild process
 * create browser instances that are not children of the current vim

This is essentially what Process#detach does when it works correctly
@guns

This comment has been minimized.

Copy link
Author

guns commented Jan 7, 2011

Okay, one solution: just do what Process#detach would have done by hand. Double fork so that the browser process is no longer a child of vim, and let the first child set up the reaping thread.

Since the first child just forks and detaches the browser process, it will return very quickly and we can feel free to block and reap it via Process#wait.

Tell me what you think!

@greyblake

This comment has been minimized.

Copy link

greyblake commented on 73446b4 Jan 7, 2011

A little hacky, but I like it:)
Great job! Thanks.

@greyblake

This comment has been minimized.

Copy link
Owner

greyblake commented Jan 7, 2011

closed

This issue was closed.

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