Skip to content
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

Fix zsh stalled $RANDOM variable #247

Merged
merged 1 commit into from
Apr 3, 2020
Merged

Fix zsh stalled $RANDOM variable #247

merged 1 commit into from
Apr 3, 2020

Conversation

mcornella
Copy link
Contributor

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: #198 (comment)

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
@mcornella mcornella mentioned this pull request Sep 11, 2018
@rupa
Copy link
Owner

rupa commented Sep 13, 2018

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
Copy link
Contributor Author

mcornella commented Sep 13, 2018

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

@mcornella
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

  • 2 _z_precmd executions are run almost simultaneously (due to two consecutive enter keypresses, leading to simultaneous prompt redraws)
  • Both 1st and 2nd executions read the .z file and compute the new values into a tempfile
  • Whichever awk computation ends last gets to overwrite .z, having no regard for the other execution's refreshed .z.

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
Copy link
Owner

rupa commented Oct 5, 2018

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@matt-harvey
Copy link

Any reason not to merge this @rupa ?

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

@rupa rupa merged commit bbec3cb into rupa:master Apr 3, 2020
@matt-harvey
Copy link

Thanks! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Database is often lost
3 participants