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

Patched random ACCESS DENIED error on Windows #1811

Merged
merged 1 commit into from
Jun 15, 2021
Merged

Conversation

Hydrocharged
Copy link
Contributor

This patches the random ACCESS DENIED error that may occur on Windows when noms attempts to rename the manifest.

@Hydrocharged
Copy link
Contributor Author

Left a massive comment in the code, so refer to that. But I'll add that I tried many, many different solutions. I cannot find the root cause of the issue. However, my patch does appear to fix it. To test, I imported a 10,000,000 statement file, and I set GC to run after every statement. On average I would get the ACCESS DENIED error every 1,500 renames, although I've seen it occur after as few as 200 renames and as many as 7,000 renames. With the patch, I was able to import the entire 10,000,000 statement file, so I'm fairly confident that this "fixes" it, although it's impossible to truly know without knowing the root cause.

Also, I've got no idea how to test this. Including a 1GB file isn't feasible, and the variability just means we're acquiring a confidence factor rather than a true test.

@Hydrocharged Hydrocharged force-pushed the daylon/file-error branch 2 times, most recently from 02c7ca4 to 8ef79fd Compare June 15, 2021 09:55
@Hydrocharged
Copy link
Contributor Author

Changed this to instead use exponential backoff. Didn't run it the whole 10,000,000 statements but I feel confident enough that it would have passed it. Whatever is causing the failure seems to disappear after even a 1ms delay for the retry, so the additional delays are a safety net. This doesn't leave the repo in a potentially invalid state that would require manual fixing.

Copy link
Contributor

@reltuk reltuk left a comment

Choose a reason for hiding this comment

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

Love it!

Copy link
Sponsor Contributor

@timsehn timsehn left a comment

Choose a reason for hiding this comment

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

Spelling mistake in comment

go/store/nbs/file_manifest.go Show resolved Hide resolved
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.

None yet

3 participants