Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Ensure shims don't temporarily disappear when rehashed #170

Closed
wants to merge 1 commit into from

3 participants

@jeremy
Collaborator

Starting a process that uses a shim during rbenv rehash will break in subtle ways, like falling back on the next one in PATH, because the shims dir is wiped then recreated. This happens fairly often since rbenv init rehashes.

This fix leaves the existing shims in place, overwrites them, then removes stale ones.

Another approach would be to build a fresh shims dir, atomically symlink it, then delete the old one.

@josh
@sstephenson
Owner

Great find Jeremy. rbenv rehash guards against concurrent invocations of itself, but doesn't account for other commands that use the shims it creates.

Since rehash runs every time you open a new shell, I'd prefer to avoid those 7 additional forks. I'll take a shot at an approach that uses only built-in commands and we can benchmark them.

@sstephenson
Owner

It turns out that using shell built-ins has a negligible performance advantage over shelling out to comm, ls, xargs and sort. But there's a different problem with the approach in this pull request: shims created by plugins such as rbenv-gemset and rbenv-bundler will still temporarily be removed during rehash.

Here's a branch that uses registration to keep track of which shims are fresh and which are stale: https://github.com/sstephenson/rbenv/compare/in-place-rehash-with-registration

@sstephenson
Owner

For the record, here are the time measurements from my laptop from three successive runs of rbenv rehash for each of master, in-place-rehash, and in-place-rehash-with-registration.

With 10 shims

master:

real    0m0.046s
user    0m0.017s
sys 0m0.028s

real    0m0.043s
user    0m0.017s
sys 0m0.026s

real    0m0.046s
user    0m0.017s
sys 0m0.027s

in-place-rehash:

real    0m0.044s
user    0m0.017s
sys 0m0.027s

real    0m0.044s
user    0m0.017s
sys 0m0.027s

real    0m0.044s
user    0m0.017s
sys 0m0.027s

in-place-rehash-with-registration:

real    0m0.044s
user    0m0.017s
sys 0m0.026s

real    0m0.044s
user    0m0.017s
sys 0m0.027s

real    0m0.044s
user    0m0.017s
sys 0m0.026s

With ~250 shims

$ mkdir -p ~/.rbenv/versions/test/bin
$ touch ~/.rbenv/versions/test/bin/{a,b,c}{a,b,c}{a,b,c}{a,b,c}{a,b,c}
$ chmod +x ~/.rbenv/versions/test/bin/*

master:

real    0m0.361s
user    0m0.129s
sys 0m0.222s

real    0m0.388s
user    0m0.135s
sys 0m0.244s

real    0m0.384s
user    0m0.133s
sys 0m0.241s

in-place-rehash:

real    0m0.362s
user    0m0.128s
sys 0m0.224s

real    0m0.412s
user    0m0.140s
sys 0m0.258s

real    0m0.417s
user    0m0.140s
sys 0m0.263s

in-place-rehash-with-registration:

real    0m0.363s
user    0m0.129s
sys 0m0.224s

real    0m0.386s
user    0m0.133s
sys 0m0.243s

real    0m0.389s
user    0m0.135s
sys 0m0.244s
@jeremy
Collaborator
@sstephenson
Owner

Agreed re: migration. Symlinking a fresh shims directory would be less code, but I think this approach is "cleaner" in that it only touches the filesystem when necessary.

Anyway, I'll merge—thanks for reviewing!

@jeremy
Collaborator
@sstephenson
Owner

I was thinking that, too. I'll add that option for 0.3.0.

@mgrubb mgrubb referenced this pull request from a commit
Commit has since been removed from the repository and is no longer available.
@cehoffman cehoffman referenced this pull request from a commit in cehoffman/rbenv
@sstephenson Add --no-rehash option to rbenv-init (#170) a53ddd2
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 23, 2011
  1. @jeremy
This page is out of date. Refresh to see the latest.
Showing with 11 additions and 4 deletions.
  1. +11 −4 libexec/rbenv-rehash
View
15 libexec/rbenv-rehash
@@ -55,17 +55,24 @@ make_shims() {
done
}
-# Save the working directory.
-CUR_PATH=$PWD
+# Remove shims that no longer reference an executable in the glob.
+remove_stale_shims() {
+ local glob="$@"
+
+ for file in $(comm -23 <(ls -1 | xargs basename | sort -u) <(ls -1 $glob | xargs basename | sort -u)); do
+ rm $file
+ done
+}
-# Empty out the shims directory and make it the working directory.
-rm -f "$SHIM_PATH"/*
+# Save the working directory and move to the shims directory.
+CUR_PATH=$PWD
cd "$SHIM_PATH"
# Create the prototype shim, then make shims for all known binaries.
create_prototype_shim
shopt -s nullglob
make_shims ../versions/*/bin/*
+remove_stale_shims ../versions/*/bin/*
# Restore the previous working directory.
cd "$CUR_PATH"
Something went wrong with that request. Please try again.