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

ido/fix crash on windows #120

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Conversation

idosavion
Copy link

@idosavion idosavion commented Aug 26, 2024

During Shrink, we try to move the temporary file which was created to the database's original location,
this may fail on windows because if you try to rename a file to a name that already exists, the operation will fail with an "Access is denied" error (while succeed and overwrite the file on mac and linux).

Additionally, fixed a small typo in test

@idosavion
Copy link
Author

@tidwall, first of all, thanks for maintaining this project!
Another approach would be to first try to perform the renaming and only delete the file and retry on failure.
I think this one is more straight forward, but let me know if you prefer the one suggested here.

@tidwall
Copy link
Owner

tidwall commented Sep 3, 2024

Can we do an optimistic check first and test for windows?

Such as :

var err error
if err = os.Rename(src, dest); err != nil {
    if os == WINDOWS {
        if err = os.Remove(dest); err == nil {
            err = os.Rename(src, dest);
        }
    }
}
return err;

This way we will effectively keep the same syscall operations on posix systems, and it won't break the atomicity guarantees with moving and unlinking files that are opened by multiple processes.

Ido Savion added 2 commits September 4, 2024 13:47
Changing the behavior to support buntdb across different operating systems
as the following crash encountered on windows while renaming in shrink:

```
panic: buntdb: rename mydb.tmp mydb: Access is denied
```
@idosavion
Copy link
Author

Thanks for the quick response @tidwall! I confirmed the (modified) fix on windows

@tidwall tidwall merged commit 3daff4e into tidwall:master Sep 10, 2024
1 check passed
@tidwall
Copy link
Owner

tidwall commented Sep 10, 2024

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.

2 participants