Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(MmapFile): Close the fd before deleting the file #242

Merged
merged 2 commits into from
Jan 8, 2021

Conversation

jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Jan 8, 2021

On windows, a file cannot be deleted if it has an open file descriptor.


This change is Reviewable

jarifibrahim pushed a commit to dgraph-io/dgraph that referenced this pull request Jan 8, 2021
There were two issues on windows
1. We were trying to delete a file with open file descriptors. This
doesn't work on windows. The PR
dgraph-io/ristretto#242 in ristretto fixes this.
2. We were using path.Join instead of filepath.Join. The path package is
supposed to be used only on unix systems. In windows the "\" is the path
separator instead of "/".
Copy link
Contributor

@NamanJain8 NamanJain8 left a comment

Choose a reason for hiding this comment

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

LGTM

@jarifibrahim jarifibrahim merged commit b1486d8 into master Jan 8, 2021
@jarifibrahim jarifibrahim deleted the ibrahim/mmap-close branch January 8, 2021 14:07
jarifibrahim pushed a commit to dgraph-io/dgraph that referenced this pull request Jan 14, 2021
There were two issues on windows

1. We were trying to delete a file with open file descriptors. This
doesn't work on windows. The PR dgraph-io/ristretto#242 in ristretto fixes this.
2. We were using path.Join instead of filepath.Join. The path package is
supposed to be used only on Unix systems. In windows the "" is the path
separator instead of "/".
jarifibrahim pushed a commit to dgraph-io/dgraph that referenced this pull request Jan 15, 2021
There were two issues on windows

1. We were trying to delete a file with open file descriptors. This
doesn't work on windows. The PR dgraph-io/ristretto#242 in ristretto fixes this.
2. We were using path.Join instead of filepath.Join. The path package is
supposed to be used only on Unix systems. In windows the "" is the path
separator instead of "/".

(cherry picked from commit 5aec243)
jarifibrahim pushed a commit to dgraph-io/dgraph that referenced this pull request Jan 15, 2021
There were two issues on windows

1. We were trying to delete a file with open file descriptors. This
doesn't work on windows. The PR dgraph-io/ristretto#242 in ristretto fixes this.
2. We were using path.Join instead of filepath.Join. The path package is
supposed to be used only on Unix systems. In windows the "" is the path
separator instead of "/".

(cherry picked from commit 5aec243)
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants