-
Notifications
You must be signed in to change notification settings - Fork 634
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 readonly file mapping on windows #307
Conversation
Can someone double-check this on windows? Thanks. I have no access to windows machine unfortunately. |
Tried to open a 500mb read-only db and I received an out-of-memory issue. Using this patch and it worked fine. Windows 11 x64, go 1.18.2 |
This change looks right, I believe the the big issue from #252 is that the high and low parameters are inverted/swapped. I'll see if I can test it locally but I'm pretty sure that the code in master is never working since those parameters are passed incorrectly. |
if h == 0 { | ||
return os.NewSyscallError("CreateFileMapping", errno) | ||
} | ||
|
||
// Create the memory map. | ||
addr, errno := syscall.MapViewOfFile(h, syscall.FILE_MAP_READ, 0, 0, uintptr(sz)) | ||
addr, errno := syscall.MapViewOfFile(h, syscall.FILE_MAP_READ, 0, 0, 0) | ||
if addr == 0 { | ||
return os.NewSyscallError("MapViewOfFile", errno) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can leak handle here, see edsrzf/mmap-go@82d537b
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unrelated to the PR, but I have added a commit with fix. Uses syscall
package instead of windows
one, to make it similar with the following code.
FWIW, mmap-go also passes https://github.com/edsrzf/mmap-go/blob/main/mmap_windows.go#L56-L59 And the docs for https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-createfilemappinga The docs for https://learn.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-mapviewoffile
|
`CreateFileMapping` tries to extend the file to the size of mapping which leads to failure if database was opened in readonly and calculated mmap size is bigger than the file size. Providing 0 to `MapViewOfFile` will create a view which has size of mapping, i.e. file-size in read-only mode and full size if file was truncated. Also, swap `sizehi` and `sizelo` names to reflect windows API docs. This was changed in 1c97a49 for seemingly no reason. Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
mmap-go does this, see edsrzf/mmap-go@82d537b Signed-off-by: Evgenii Stratonikov <evgeniy@nspcc.ru>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thank you @fyrchik
@fyrchik Please add a changelog item in CHANGELOG.md |
I have encountered a problem where my DB won't open on windows in readonly mode. This seems to be related to #252 as databases there also have bad size.
CreateFileMapping
tries to extend the file to the size of mappingwhich leads to failure if database was opened in readonly and calculated
mmap size is bigger than the file size.
Providing 0 to
MapViewOfFile
will create a view which has size ofmapping, i.e. file-size in read-only mode and full size if file was
truncated.
Also, swap
sizehi
andsizelo
names to reflect windows API docs.This was changed in 1c97a49 for seemingly no reason.
I have tested this on my windows 8 x64 machine, my case has been resolved.
@lunemec, I wonder if this fixes your issue too.