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

Retry rename on EACCES #1386

Merged
merged 1 commit into from Jan 6, 2022
Merged

Retry rename on EACCES #1386

merged 1 commit into from Jan 6, 2022

Conversation

aantron
Copy link
Contributor

@aantron aantron commented Jan 1, 2022

This PR retries rename upon getting EACCES. I've included data about how many retries are likely necessary.

On my system (WSL1 Ubuntu 20.04, omitting hardware details), the EACCES issue makes it impossible to use esy to install any of the Dream examples or use Dream's quick start. As the data below shows, every installation is expected to fail with EACCES, if it is not worked around.

The underlying EACCES issue seems to be a long-standing problem on WSL1 (microsoft/WSL#1529, microsoft/WSL#3395), and I think we do have to work around it in esy. I'm not sure what is causing the EACCES exactly. I think there are two main classes of possibilities:

  • Self-interaction between esy's opened file descriptors and rename. I think the self-interaction is due to WSL rather than Lwt or another library. Since I compiled esy on WSL, it is using Lwt's Unix (rather than Windows) C code. Since the Unix code seems to work fine on Linux and Mac, this suggests a WSL issue.
  • Interaction between esy and file indexers or other proceses running on the system. I'm not sure if that's a WSL issue or not, but I've never had to be aware of such processes when doing renames in Cygwin or elsewhere.

I built esy with this patch under WSL and ran clean esy installs in Dream's full-stack ReScript example, using this script:

#!/bin/bash

export PATH="/home/antron/code/attic/esy/_build/install/default/bin:$PATH"
export ESY__PREFIX="/home/antron/code/dream/dream/example/w-fullstack-rescript/esy-prefix"
export OCAMLRUNPARAM=b
RUN=1

while true
do
  rm -rf esy-prefix _esy esy.lock lib node_modules/ package-lock.json
  echo
  echo "RUN $RUN"
  which esy
  esy install # --verbosity debug
  if [ $? != 0 ]
  then
    exit
  fi
  RUN=$((RUN+1))
done

The example was checked out into NTFS. The system was freshly restarted, and VSCode (or anything similar) was not running.

I used a version of this patch with a print showing the number of attempts before rename succeeds, and got the following results from 5 runs:

1 attempt:   802
2 attempts:   52
3 attempts:   12
4 attempts:    3
5 attempts:    1
total:       870

Based on this, I naively estimated that if a rename needs more than 1 attempt, the number of attempts needed decays by a factor of 4 at each step. I set the limit on the number of attempts naively to 8, thus expecting one failure in about 500 esy install attempts of the Dream ReScript example, under all these simplified assumptions.

The delay between attempts is (over) one second, so this means that upon legitimate EACCES, users will have to wait eight seconds to get an error message. I think there are two ways to address this:

  • Fall back to recursive copy rather than retrying rename when rename fails. Do we have a recursive copy available in esy or its dependencies? Is it fine to leave the source directory intact?
  • Detect WSL and retry only on WSL. Waiting for 8 seconds is still a much better user experience than failure to install at all, so the PR will still be an improvement, without, in this case, harming Linux or Mac users. We could also add a message, shown in case we finally fail with EACCES on WSL, giving users a hint about potential VSCode or other watchers, and what else they can try to solve the problem.

Closes #1363.
Probably fixes #1097, some of the reports after the first one.
Probably fixes #1083.
Probably fixes #593, but I haven't looked into non-WSL Windows yet.
Probably fixes aantron/dream#63.

cc @bryphe, @rizo, @jordwalke, @iMplode-nZ, @a-c-sreedhar-reddy, @srirajshukla, @andreypopp

@ManasJayanth
Copy link
Member

@aantron Thank you for putting all this effort into figuring the retry limit - something I couldn't get around to doing in #1363 Patch looks good to me as I have tried something similar myself.

cc @melwyn95 @EduardoRFS Would one of you like give it another look?

@aantron
Copy link
Contributor Author

aantron commented Jan 5, 2022

I've been using this patched esy since this PR. It seems to work fine.

Copy link
Contributor

@melwyn95 melwyn95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch looks good to me as well, Thanks @aantron

@ManasJayanth ManasJayanth merged commit 3e230b2 into esy:master Jan 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants