rupa / z

z - jump around
Do What The F*ck You Want To Public License
16.38k stars 1.17k forks source link

Fix zsh stalled $RANDOM variable #247

Closed mcornella closed 4 years ago

mcornella commented 6 years ago

This change references $RANDOM outside the subshell to refresh it for the next subshell invocation. Otherwise, subsequent runs of the function get the same value and, if run simultaneously, they may clobber each others' temp .z files.

This is due to how zsh distributes RANDOM values when running inside a subshell:

subshells that reference RANDOM will result in identical pseudo-random
values unless the value of RANDOM is referenced or seeded in the parent
shell in between subshell invocations

See: http://zsh.sourceforge.net/Doc/Release/Parameters.html#index-RANDOM

Fixes #198 Related: https://github.com/rupa/z/issues/198#issuecomment-417098496

rupa commented 6 years ago

i really wish the solution to this involved seeding $RANDOM, and not sure why that isn't practical. if you could say some words about why seeding is a pain, i'd appreciate it

mcornella commented 6 years ago

I haven't really tried the seeding option, I'll get to you with an answer soon.

mcornella commented 6 years ago

So the thing about seeding $RANDOM is that we have to seed it with a changing value, otherwise we're just resetting the PRNG. A solution could be using the timestamp, but that requires using milliseconds. If we used seconds, we could be running multiple z commands with the same RANDOM value again when pressing the enter key repeatedly.

If we have to use milliseconds in zsh, without external dependencies, we'd have to use EPOCHREALTIME and load the module zsh/datetime. Case in point:

testrand() {
  zmodload zsh/datetime;
  (echo $RANDOM);
  RANDOM=$(( EPOCHREALTIME * 1000 ))
}
$  testrand; testrand; testrand; testrand; testrand
23846
26329
11760
6861
25030

I haven't found any other solutions to seeding RANDOM that work or are feasible. So I think just referencing it and letting zsh gives us sequencial values is the best option.

mcornella commented 6 years ago

Wait, we could also re-seed it using $RANDOM:

testrand() { (echo $RANDOM); RANDOM=$RANDOM }
$ testrand; testrand; testrand; testrand; testrand
32401
16247
21221
10218
8481

This option looks very much like my first suggestion. And it actually both references and seeds RANDOM. But using this one over the other is just a matter of personal preference.

mcornella commented 6 years ago

I'm not sure this PR will fix the underlying race condition though, which is the following:

To fix this race condition we should go back to having a synchronous precmd function.

It's not as important a use case to warrant switching to the old ways though...

rupa commented 6 years ago

not sure it will help your thinking but in case it does: keep in mind it's not a huge deal if a race condition swallows a keypress or two and the .z file isn't perfectly accurate - things should still work fine if we get 99% of data. the real important thing is not to have a race condition that clobbers the whole file.

mcornella commented 6 years ago

Right, that's what I thought. So what should we do with this PR? Do you want me to change it to the reseed version?

mcornella commented 4 years ago

This has been running successfully in Oh My Zsh for over a year now.

matt-harvey commented 4 years ago

Any reason not to merge this @rupa ?

I don't want to use Oh My Zsh, but I do want this fix.

matt-harvey commented 4 years ago

Thanks! :)