Skip to content

Foundation: port NSData to Windows #1868

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Foundation: port NSData to Windows #1868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Foundation/Data.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

#if DEPLOYMENT_RUNTIME_SWIFT

#if os(Linux)
#if os(Linux) || os(Windows)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this be better as a #if !canImport(Darwin) as I dont think it is supported on any other platforms apart from Apple's?

Copy link
Contributor

Choose a reason for hiding this comment

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

That is true. The only other malloc implementation that has this feature iirc is jemalloc.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be interesting to be able to use jemalloc on other targets. I know that there are environments running off of jemalloc. I don't think that we have a good way to identify that yet. Perhaps having a macro from the build system -DHAVE_MALLOC_GOOD_SIZE?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think swift just needs a #if hasImported(<some function>) conditional :)

@inlinable // This is @inlinable as trivially computable.
internal func malloc_good_size(_ size: Int) -> Int {
return size
Expand All @@ -22,7 +22,11 @@ internal func malloc_good_size(_ size: Int) -> Int {
import CoreFoundation

internal func __NSDataInvokeDeallocatorUnmap(_ mem: UnsafeMutableRawPointer, _ length: Int) {
#if os(Windows)
UnmapViewOfFile(mem)
#else
munmap(mem, length)
#endif
}

internal func __NSDataInvokeDeallocatorFree(_ mem: UnsafeMutableRawPointer, _ length: Int) {
Expand Down
63 changes: 62 additions & 1 deletion Foundation/NSData.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,6 +435,24 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {

internal func makeTemporaryFile(inDirectory dirPath: String) throws -> (Int32, String) {
let template = dirPath._nsObject.appendingPathComponent("tmp.XXXXXX")
#if os(Windows)
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldnt this whole function just be replaced with:
return try _NSCreateTemporaryFile(dirPath._nsObject.appendingPathComponent("")
or something like that, they seem to have the same bodies of code expect for computing the initial template.

Copy link
Member Author

Choose a reason for hiding this comment

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

I honestly don't know. I'm not sure why this is duplicated in the first place, as the same thing happens in the Linux case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, looking at this, no, it wouldn't work, since that would alter the template and give you a different result.

Copy link
Contributor

Choose a reason for hiding this comment

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

What about passing the template as the argument, eg:

internal func _NSCreateTemporaryFile(withTemplate template: String)

There only seems to be one caller of each method so generating the template at the call-site seems to solve the issue.

let maxLength = Int(MAX_PATH) + 1
var buf: [UInt16] = Array<UInt16>(repeating: 0, count: maxLength)
let length = GetTempPathW(DWORD(MAX_PATH), &buf)
precondition(length <= MAX_PATH - 14, "temp path too long")
if "SCF".withCString(encodedAs: UTF16.self, {
return GetTempFileNameW(buf, $0, 0, &buf)
}) == FALSE {
throw _NSErrorWithErrno(Int32(GetLastError()), reading: false,
path: dirPath)
}
let pathResult =
FileManager.default
.string(withFileSystemRepresentation: String(decoding: buf,
as: UTF16.self),
length: wcslen(buf))
let fd = open(pathResult, _O_CREAT)
#else
let maxLength = Int(PATH_MAX) + 1
var buf = [Int8](repeating: 0, count: maxLength)
let _ = template._nsObject.getFileSystemRepresentation(&buf, maxLength: maxLength)
Expand All @@ -443,13 +461,14 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
throw _NSErrorWithErrno(errno, reading: false, path: dirPath)
}
let pathResult = FileManager.default.string(withFileSystemRepresentation:buf, length: Int(strlen(buf)))
#endif
return (fd, pathResult)
}

internal class func write(toFileDescriptor fd: Int32, path: String? = nil, buf: UnsafeRawPointer, length: Int) throws {
var bytesRemaining = length
while bytesRemaining > 0 {
var bytesWritten : Int
var bytesWritten: Int = 0
repeat {
#if os(macOS) || os(iOS)
bytesWritten = Darwin.write(fd, buf.advanced(by: length - bytesRemaining), bytesRemaining)
Expand All @@ -465,6 +484,22 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
}
}

#if os(Windows)
internal class func write(toHandle handle: HANDLE, path: String? = nil, buf: UnsafeRawPointer, length: Int) throws {
var BytesToWrite = length
while BytesToWrite > 0 {
var BytesWritten: DWORD = 0
if WriteFile(handle, buf.advanced(by: length - BytesToWrite), DWORD(BytesToWrite), &BytesWritten, nil) == FALSE {
throw _NSErrorWithErrno(Int32(GetLastError()), reading: false, path: path)
}
if BytesWritten == 0 {
throw _NSErrorWithErrno(Int32(GetLastError()), reading: false, path: path)
}
BytesToWrite -= Int(BytesWritten)
}
}
#endif

/// Writes the data object's bytes to the file specified by a given path.
open func write(toFile path: String, options writeOptionsMask: WritingOptions = []) throws {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to try and rewrite this whole method using FileHandle and eliminate all of the Windows/Unix differences here. It will requires some new internal FileHandle methods (eg a getFilePermissions, setFilePermissions, and a throwable version of synchronizeFile) but I think it should be possible.
makeTemporaryFile could return a FileHandle instead of an fd as well. What do you think?

The helper methods could probably be reused in FileManager later on any

Copy link
Member Author

Choose a reason for hiding this comment

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

I like that idea, if that is acceptable as per @parkera, @phausler, or @millenomi. The problem is that this is a public interface, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I just meant change the function body. Would something like this work (untested, and you will need to add an internal FileHandle.setFilePermissions(mode) method:

open func write(toFile path: String, options writeOptionsMask: WritingOptions = []) throws {
        let fm = FileManager.default

        let fh: FileHandle
        let useAuxiliaryFile = writeOptionsMask.contains(.atomic)
        var auxFilePath: String? = nil
        var mode: mode_t? = nil

        if useAuxiliaryFile {
            if let perms = try? fm.attributesOfItem(atPath: path)[.posixPermissions] as? NSNumber, let m = perms {
                mode = mode_t(m.uintValue)
            }

            let (newFD, path) = try self.makeTemporaryFile(inDirectory: path._nsObject.deletingLastPathComponent)
            fh = FileHandle(fileDescriptor: newFD, closeOnDealloc: true)
            auxFilePath = path
        } else {
            var flags = O_WRONLY | O_CREAT | O_TRUNC
            if writeOptionsMask.contains(.withoutOverwriting) {
                flags |= O_EXCL
            }

            guard let newfh = FileHandle(path: path, flags: flags, createMode: 0o666) else {
                throw _NSErrorWithErrno(errno, reading: false, path: path)
            }
            fh = newfh
        }

        try self.enumerateByteRangesUsingBlockRethrows { (buf, range, stop) in
            if range.length > 0 {
                do {
                    try NSData.write(toFileDescriptor: fh.fileDescriptor, path: path, buf: buf, length: range.length)
                    fh.synchronizeFile()
                } catch {
                    if let auxFilePath = auxFilePath {
                        try? fm.removeItem(atPath: auxFilePath)
                    }
                    throw error
                }
                fh.setFilePermissions(mode) // TODO: Add this as an internal Filehandle function 
            }
        }

        if let auxFilePath = auxFilePath {
            try fm._fileSystemRepresentation(withPath: path, { pathFsRep in
                try fm._fileSystemRepresentation(withPath: auxFilePath, { auxFilePathFsRep in
                    if rename(auxFilePathFsRep, pathFsRep) != 0 {
                        try? FileManager.default.removeItem(atPath: auxFilePath)
                        throw _NSErrorWithErrno(errno, reading: false, path: path)
                    }
                    if let mode = mode {
                        chmod(pathFsRep, mode)
                    }
                })
            })
        }
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

I can rewrite this in terms of FileHandle, but note that there will still be Windows specific paths here as FileHandle.fileDescriptor is not available on Windows, since it works in terms of HANDLEs. Additionally, a few of the operations that this is performing would need to be moved around (e.g. chmod). The makeTemporaryFile will still require Windows specific handling (FileHandle wants a HANDLE, not a file descriptor).

Copy link
Contributor

Choose a reason for hiding this comment

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

What about moving the two internal NSData.write() functions to be internal methods of FileHandle (its the only caller of them anyway, and it would match _readDataOfLength()) then you should be able to eliminate the fh.fileDescriptor.

Copy link
Member Author

Choose a reason for hiding this comment

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

IMO, that is far more accessible for developers working on FileHandle. I remember looking at this and getting confused why it was split. @parkera @phausler - wanna chime in?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the only reason why this was two separate implementations is because they are transliterations from the objc implementations. The implementations for NSData live in CoreFoundation and NSFileHandle lives in Foundation. Plus from a historical case the implementations for NSFileHandle use NSExceptions heavily (whereas NSData does not). Frankly these are all implementation details that probably dont need to be maintained across to SCLF. It makes sense to me that file handle things like writing etc should be handled by FileHandle. @millenomi might have some insight here on which way to favor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ive done a refactor of the FileHandle write paths in #1876

let fm = FileManager.default
Expand All @@ -474,17 +509,37 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
let useAuxiliaryFile = writeOptionsMask.contains(.atomic)
var auxFilePath : String? = nil
if useAuxiliaryFile {
#if os(Windows)
var info: ucrt._stat64 = ucrt._stat64()
try! String(cString: pathFsRep).withCString(encodedAs: UTF16.self) { wpath in
if ucrt._wstat64(wpath, &info) == 0 {
let mode = mode_t(info.st_mode)
} else if errno != ENOENT && errno != ENAMETOOLONG {
throw _NSErrorWithErrno(errno, reading: false, path: path)
}
}
#else
// Preserve permissions.
var info = stat()
if lstat(pathFsRep, &info) == 0 {
let mode = mode_t(info.st_mode)
} else if errno != ENOENT && errno != ENAMETOOLONG {
throw _NSErrorWithErrno(errno, reading: false, path: path)
}
#endif
let (newFD, path) = try self.makeTemporaryFile(inDirectory: path._nsObject.deletingLastPathComponent)
fd = newFD
auxFilePath = path
#if os(Windows)
let handle: HANDLE = HANDLE(bitPattern: _get_osfhandle(fd))!
var buf = [UInt16](repeating: 0, count: Int(MAX_PATH + 1))
if GetFinalPathNameByHandleW(handle, &buf, DWORD(MAX_PATH), DWORD(FILE_NAME_NORMALIZED)) == FALSE {
fatalError("GetFinalPathNameByHandle() failed")
}
_wchmod(buf, 0o666)
#else
fchmod(fd, 0o666)
#endif
} else {
var flags = O_WRONLY | O_CREAT | O_TRUNC
if writeOptionsMask.contains(.withoutOverwriting) {
Expand All @@ -503,9 +558,15 @@ open class NSData : NSObject, NSCopying, NSMutableCopying, NSSecureCoding {
if range.length > 0 {
do {
try NSData.write(toFileDescriptor: fd, path: path, buf: buf, length: range.length)
#if os(Windows)
if FlushFileBuffers(HANDLE(bitPattern: _get_osfhandle(fd))) == FALSE {
throw _NSErrorWithErrno(Int32(GetLastError()), reading: false, path: path)
}
#else
if fsync(fd) < 0 {
throw _NSErrorWithErrno(errno, reading: false, path: path)
}
#endif
} catch {
if let auxFilePath = auxFilePath {
try? FileManager.default.removeItem(atPath: auxFilePath)
Expand Down