Add throttling to postinstall #93
Conversation
|
I think the delay should be reduced. 3 or 2 seconds should work fine. When I tested with 1 second it spawned the reshim more than once. |
|
Actually |
| @@ -1,3 +1,42 @@ | |||
| #!/usr/bin/env bash | |||
|
|
|||
| asdf reshim nodejs "$ASDF_INSTALL_VERSION" | |||
| set -e | |||
There was a problem hiding this comment.
We might consider removing set -e.
There was a problem hiding this comment.
I like being strict when it comes to scripts like this.
Previous implementation had some corner case bugs. New implementation is much simpler and the comments were added.
This value should still prevent reshim from being called more than once.
| set -e | ||
|
|
||
| declare LOCK_FILE="/tmp/asdf-nodejs-postinstall.lock.2ac8ccc4" | ||
| declare LAST_CALLED_FILE="/tmp/asdf-nodejs-postinstall.last_called.dd05fa02" |
There was a problem hiding this comment.
These paths should be dynamic. We should probably use mktemp for these files.
There was a problem hiding this comment.
We need one common lock file and one common last_called file. We can check if /tmp/asdf-nodejs-postinstall.last_called.* exists and use the existing one if it does, generating it otherwise.
But we would have to run it in the foreground thus making it a little slower.
Actually, I think we can even safely drop the .2ac8ccc4 and .dd05fa02 suffixes, it's just me being a little paranoic.
There was a problem hiding this comment.
Yes you can get rid of those hashes if there is only going to be at most one file at a time.
|
Was this code copied from somewhere? Seems like most of it is pretty generic, except for that one line containing the reshim call. If it was copied from somewhere please cite the source so we can verify the license is compatible with ours. Overall I think this is great. I'm curious if this could cause problems in shell scripts though. Suppose a shell script installs something and then immediately tries to use it. |
|
Yes, I'm aware that there will be some delay between installation and reshim. But I only thought about using in it shell prompt. I think it would cause a lot of problems with scripts like your example so don't merge it right now. And I thought of a possibly better solution. Why not just call reshim in The version in the first commit is based on the idea from this gist. But I've rewritten and simplified the code in the second commit and it uses a little different idea now. |
|
@MrQubo @Stratus3D If it fixes the Also do you know how NVM handles this issue? |
|
A bunch of asdf plugins e.g. haskell, ocaml, python, elixir do not call reshim and mentions that reshim has to be called separately after installing new scripts/packages/binaries. |
|
@MrQubo possibly, or we may stick with the slow current implementation until a better solution is found. I would be interested to know how nvm solves this issue. It seems like this is an issue all the version managers would face. I'm going to close this PR. |
Installing any global packages is currently VERY slow.
It works by spawning the process in the background on every execution of the hook. It executes the reshim only if it's the last hook call within the last 10 seconds.
I've tested this one on Arch Linux. It works well and reduces the execution times for individual hook to the minimum.
You can test it by adding
echo ASDF RESHIM NODEJSaboveasdf reshimcommand. Then executenpm i -g npm, it will call the hook a few hundred times, but the echo message should be displayed only once (with 10-second delay after last hook call).