ronantakizawa/codereview-bench huggingface dataset:
Repository: vapor/vapor
File: Sources/HTTP/Streams/Server.swift (Swift)

Code before:
import Core
import TCP

/// HTTP server wrapped around TCP server
public final class Server<S: OutputStream>: Core.OutputStream where S.Output == TCP.Client {
    // MARK: Stream
    public typealias Output = HTTP.Client
    public var errorStream: ErrorHandler? {
        get { return server.errorStream }
        set { server.errorStream = newValue }
    }

    public var outputStream: OutputHandler?

    public let server: S

    public init(server: S) {
        self.server = server
        server.outputStream = { tcp in
            let client = HTTP.Client(tcp: tcp)
            self.outputStream?(client)
        }
    }
}


Review comment (suggestion):
I don't think the HTTP should need to be affected by this change. I think we can do this totally at the TCP layer. (Or we do it totally at the HTTP layer, either way. But touching both layers seems excessive)

Code after:
import Async
import TCP

/// HTTP server wrapped around TCP server
public final class Server<ClientStream: OutputStream>: Async.OutputStream where ClientStream.Output == TCPClient {
    // MARK: Stream
    public typealias Output = HTTP.Peer
    
    /// See `BaseStream.errorStream`
    public var errorStream: ErrorHandler? {
        get { return clientStream.errorStream }
        set { clientStream.errorStream = newValue }
    }

    /// See `OutputStream.outputStream`
    public var outputStream: OutputHandler?

    /// The wrapped Client Stream
    public let clientStream: ClientStream

    /// Creates a new HTTP Server from a Client stream
    public init(clientStream: ClientStream) {
        self.clientStream = clientStream
        clientStream.outputStream = { tcp in
            let client = HTTP.Peer(tcp: tcp)
            self.outputStream?(client)
        }
    }
}
Repository: vapor/vapor
File: Sources/TCP/Socket/Socket.swift (Swift)

Code before:
import Core
import Dispatch
import libc

/// Any TCP socket. It doesn't specify being a server or client yet.
public class Socket {
    /// The file descriptor related to this socket
    public let descriptor: Descriptor

    /// The remote's address
    public var address: Address?
    
    /// True if the socket is non blocking
    public let isNonBlocking: Bool

    /// True if the socket should re-use addresses
    public let shouldReuseAddress: Bool
    
    /// Will be triggered before closing the socket, as part of the cleanup process
    public var beforeClose: (()->())? = nil

    /// Creates a TCP socket around an existing descriptor
    public init(
        established: Descriptor,
        isNonBlocking: Bool,
        shouldReuseAddress: Bool
    ) {
        self.descriptor = established
        self.isNonBlocking = isNonBlocking
        self.shouldReuseAddress = shouldReuseAddress
    }
    
    /// Creates a new TCP socket
    public convenience init(
        isNonBlocking: Bool = true,
        shouldReuseAddress: Bool = true
    ) throws {
        let sockfd = socket(AF_INET, SOCK_STREAM, 0)
        guard sockfd > 0 else {
            throw Error.posix(errno, identifier: "socketCreate")
        }
        let descriptor = Descriptor(raw: sockfd)

        if isNonBlocking {
            // Set the socket to async/non blocking I/O

Review comment (suggestion):
We should use a type-alias here to make it clearer.

much more readable imo:
```swift
public typealias BeforeCloseHandler = () -> ()
public var beforeClose: BeforeCloseHandler?
```


Code after:
import Bits
import Async
import Dispatch
import libc

/// Any TCP socket. It doesn't specify being a server or client yet.
public final class Socket {
    /// The file descriptor related to this socket
    public let descriptor: Int32

    /// The remote's address
    public var address: Address?
    
    /// True if the socket is non blocking
    public let isNonBlocking: Bool

    /// True if the socket should re-use addresses
    public let shouldReuseAddress: Bool
    
    /// A handler that gets called when closing the socket
    public typealias CloseHandler = (()->())
    
    /// Will be triggered before closing the socket, as part of the cleanup process
    public var didClose: CloseHandler? = nil
    
    /// A write source that's used to check when the connection is open
    internal var writeSource: DispatchSourceWrite?

    /// Creates a TCP socket around an existing descriptor
    public init(
        established: Int32,
        isNonBlocking: Bool,
        shouldReuseAddress: Bool,
        address: Address?
    ) {
        self.descriptor = established
        self.isNonBlocking = isNonBlocking
        self.shouldReuseAddress = shouldReuseAddress
        self.address = address
    }
    
    /// Creates a new TCP socket
    public convenience init(
        isNonBlocking: Bool = true,
        shouldReuseAddress: Bool = true
Repository: vapor/vapor
File: Sources/TCP/Utilties/Address.swift (Swift)

Code before:
        var copy = self
        
        return withUnsafePointer(to: &copy) { pointer in
            return pointer.withMemoryRebound(to: sockaddr_in.self, capacity: 1) { pointer in
                var address = pointer.pointee.sin_addr
                
                return call(&address)
            }
        }
    }
    
    // Accesses the sockaddr_storage as sockaddr_in6
    fileprivate func withIn6_addr<T>(call: ((inout in6_addr)->(T))) -> T {
        var copy = self
        
        return withUnsafePointer(to: &copy) { pointer in
            return pointer.withMemoryRebound(to: sockaddr_in6.self, capacity: 1) { pointer in
                var address = pointer.pointee.sin6_addr
                
                return call(&address)
            }
        }
    }
}

fileprivate func htons(_ value: UInt16) -> UInt16 {
    return (value << 8) + (value >> 8)
}


Review comment (suggestion):
a comment here would be nice even tho it's fileprivate. `converts host byte order to network byte order`

Code after:
            // Extract the remote IPv6 address
            
            // Allocate an IPv6 address
            maxStringLength = socklen_t(INET6_ADDRSTRLEN)
            stringData = UnsafeMutablePointer<Int8>.allocate(capacity: numericCast(maxStringLength))
            
            _ = self.storage.withIn6_addr { address in
                inet_ntop(numericCast(self.storage.ss_family), &address, stringData, maxStringLength)
            }
        default:
            // Impossible scenario
            fatalError()
        }
        
        defer {
            // Clean up
            stringData.deallocate(capacity: numericCast(maxStringLength))
        }
        
        // This cannot fail
        return String(validatingUTF8: stringData)!
    }
}

extension sockaddr_storage {
    // Accesses the sockaddr_storage as sockaddr_in
    fileprivate func withIn_addr<T>(call: ((inout in_addr)->(T))) -> T {
        var copy = self
        
        return withUnsafePointer(to: &copy) { pointer in
            return pointer.withMemoryRebound(to: sockaddr_in.self, capacity: 1) { pointer in
                var address = pointer.pointee.sin_addr
                
                return call(&address)
            }
        }
    }
    
    // Accesses the sockaddr_storage as sockaddr_in6
    fileprivate func withIn6_addr<T>(call: ((inout in6_addr)->(T))) -> T {
        var copy = self
        
        return withUnsafePointer(to: &copy) { pointer in
            return pointer.withMemoryRebound(to: sockaddr_in6.self, capacity: 1) { pointer in
                var address = pointer.pointee.sin6_addr
                
                return call(&address)
            }
        }
    }
}
Repository: vapor/vapor
File: Sources/Redis/Pipeline.swift (Swift)

Code before:
import Async

extension RedisClient {
    /// Accepts an array of commands, to be sent in one request.
    public func pipeline(_ commands: () -> ([RedisData])) -> Future<[RedisData]> {
        if isSubscribed {
            return Future(error: RedisError(.cannotReuseSubscribedClients))
        }
        
        let commands = commands()
        dataSerializer.inputStream(commands)
        
        let promises = commands.map { _ in
            Promise<RedisData>()
        }
        
        dataParser.responseQueue.append(contentsOf: promises)
        
        return promises
            .map { $0.future }
            .flatten()
    }
}


Review comment (refactor):
I'd make this a `Pipeline` object that internally has a `[RedisData]` buffer. That way we can add helpers in the future.

Code after:
import Async
import Bits
import Foundation

final class Pipeline<DuplexByteStream: Async.Stream> where DuplexByteStream.Input == ByteBuffer, DuplexByteStream.Output == ByteBuffer, DuplexByteStream: ClosableStream {
    
    let client: RedisClient<DuplexByteStream>
    private var commands = [RedisData]()
    
    init(_ client:  RedisClient<DuplexByteStream>) {
        self.client = client
    }
    
    /// Enqueues a commands.
    @discardableResult
    public func enqueue(command: String, arguments: [RedisData]? = nil) throws -> Pipeline<DuplexByteStream> {
        if client.isSubscribed {
            throw RedisError(.cannotReuseSubscribedClients)
        }
        
        commands.append(RedisData.array([.bulkString(command)] + (arguments ?? [])))
        return self
    }
    
    /// Executes a series of commands and returns a future for the responses
    @discardableResult
    public func execute() throws -> Future<[RedisData]> {
        defer {
            commands = []
        }
Repository: vapor/vapor
File: Sources/Vapor/Websocket+Router.swift (Swift)

Code before:
import WebSocket

extension Router {
    public typealias WebSocketClosure = (Request, WebSocket) throws -> Void

    @discardableResult
    public func websocket(_ path: PathComponent...,
        subprotocols: [String]? = nil,
        use closure: @escaping WebSocketClosure
    ) -> Route {
        let responder = RouteResponder { request in
            return try request.upgradeToWebSocket(subprotocols: subprotocols) { websocket in
                try closure(request, websocket)
            }
        }
        let route = Route(method: .get, path: path, responder: responder)
        self.register(route: route)

        return route
    }
}



Review comment (suggestion):
I'd prever, instead of setting the protocols in an array, to provide a `WebSocketSettings` struct with a default value. That way we can add more settings in this `.websocket()` route in the future. I'm already thinking of settings like `maxPacketSize` for configuring memory usage, only accepting `was` (secure) connections etc.

Code after:
import WebSocket

extension Router {
    /// Registers a websocket route handler at the supplied path.
    /// WebSocketSettings can be used as an [String] to define custom subprotocols.
    ///
    /// example: router.websocket("path", with: ["subprotocol"]) { req, ws in /*...*/ }
    @discardableResult
    public func websocket(_ path: PathComponent...,
        with settings: WebSocketSettings = WebSocketSettings(),
        onUpgrade closure: @escaping WebSocket.OnUpgradeClosure) -> Route {

        let responder = RouteResponder { request in
            return try WebSocket.upgradeResponse(for: request, with: settings, onUpgrade: closure)
        }
        let route = Route(method: .get, path: path, responder: responder)
        self.register(route: route)

        return route
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Websocket+Router.swift (Swift)

Code before:
import WebSocket

extension Router {
    public typealias WebSocketClosure = (Request, WebSocket) throws -> Void

    @discardableResult
    public func websocket(_ path: PathComponent...,
        subprotocols: [String]? = nil,
        use closure: @escaping WebSocketClosure
    ) -> Route {
        let responder = RouteResponder { request in
            return try request.upgradeToWebSocket(subprotocols: subprotocols) { websocket in
                try closure(request, websocket)
            }
        }
        let route = Route(method: .get, path: path, responder: responder)
        self.register(route: route)

        return route
    }
}



Review comment (suggestion):
As part of Vapor 3 we require every PR to have inline documentation for all functions, variables and types. We also require adding a PR for [the vapor docs](https://github.com/vapor/documentation) for API changes. But since we're in an early stage I'll fill this one in for you 🙂

Code after:
import WebSocket

extension Router {
    /// Registers a websocket route handler at the supplied path.
    /// WebSocketSettings can be used as an [String] to define custom subprotocols.
    ///
    /// example: router.websocket("path", with: ["subprotocol"]) { req, ws in /*...*/ }
    @discardableResult
    public func websocket(_ path: PathComponent...,
        with settings: WebSocketSettings = WebSocketSettings(),
        onUpgrade closure: @escaping WebSocket.OnUpgradeClosure) -> Route {

        let responder = RouteResponder { request in
            return try WebSocket.upgradeResponse(for: request, with: settings, onUpgrade: closure)
        }
        let route = Route(method: .get, path: path, responder: responder)
        self.register(route: route)

        return route
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Middleware/FileMiddleware.swift (Swift)

Code before:
import libc
import HTTP
import Bits
import Core
import Foundation
import Dispatch

/// Services files from the public folder.
public final class FileMiddleware: Middleware {
    /// The public directory.
    /// note: does _not_ end with a slash
    let publicDirectory: String
    
    public var webTypes = [MediaType]()

    /// Creates a new filemiddleware.
    public init(publicDirectory: String) {
        self.publicDirectory = publicDirectory.finished(with: "/")
    }

    /// See Middleware.respond.
    public func respond(to req: Request, chainingTo next: Responder) throws -> Future<Response> {
        return try next.respond(to: req).map { response in
            if response.http.status == 404 {
                let reader = try req.eventLoop.make(FileReader.self, for: FileMiddleware.self)
                
                var path = req.http.uri.path
                if path.hasPrefix("/") {
                    path = String(path.dropFirst())
                }
                guard !path.contains("../") else {
                    throw Abort(.forbidden)
                }
                
                let filePath = self.publicDirectory + path
                
                guard reader.fileExists(at: filePath) else {
                    return response
                }
                
                return try req.streamFile(at: filePath)
            } else {
                return response
            }
        }
    }
}


Review comment (bug):
we should continue checking for the file before we hit the router. this will be different than what we did in vapor 2, but it more closely resembles how nginx file serving works and will prevent bugs there. it should also improve the speed of serving files since we don't have to go through the router first. 

Code after:
import libc
import HTTP
import Bits
import Core
import Foundation
import Dispatch

/// Services files from the public folder.
public final class FileMiddleware: Middleware {
    /// The public directory.
    /// note: does _not_ end with a slash
    let publicDirectory: String
    
    public var webTypes = [MediaType]()

    /// Creates a new filemiddleware.
    public init(publicDirectory: String) {
        self.publicDirectory = publicDirectory.finished(with: "/")
    }

    /// See Middleware.respond.
    public func respond(to req: Request, chainingTo next: Responder) throws -> Future<Response> {
        let reader = try req.eventLoop.make(FileReader.self, for: FileMiddleware.self)
        
        var path = req.http.uri.path
        if path.hasPrefix("/") {
            path = String(path.dropFirst())
        }
        guard !path.contains("../") else {
            throw Abort(.forbidden)
        }
        
        let filePath = self.publicDirectory + path
        
        guard reader.fileExists(at: filePath) else {
            return try next.respond(to: req)
        }
        
        return Future(try req.streamFile(at: filePath))
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Engine/EngineServer.swift (Swift)

Code before:
    // public var ssl: EngineServerSSLConfig?

    /// Creates a new engine server config
    public init(
        hostname: String = "localhost",
        port: UInt16 = 8080,
        backlog: Int32 = 4096,
        workerCount: Int = ProcessInfo.processInfo.activeProcessorCount,
        maxConnectionsPerIP: Int = 128
    ) {
        self.hostname = hostname
        self.port = port
        self.workerCount = workerCount
        self.backlog = backlog
        self.maxConnectionsPerIP = maxConnectionsPerIP
        // self.ssl = nil
    }
}

extension EngineServerConfig {
    public static func heroku(port: UInt16=8080) -> EngineServerConfig {
        if let portString = ProcessInfo.processInfo.environment["PORT"],
            let customPort = UInt16(portString) {
            return EngineServerConfig(hostname: "0.0.0.0", port: customPort)
        }
        return EngineServerConfig(hostname: "localhost", port: port)
    }
}



Review comment (suggestion):
Heroku doesn't like `localhost` as a hostname. Even when there's no port defined, we should amend the hostname to `"0.0.0.0"`.

Code after:
    // public var ssl: EngineServerSSLConfig?

    /// Creates a new engine server config
    public init(
        hostname: String = "localhost",
        port: UInt16 = 8080,
        backlog: Int32 = 4096,
        workerCount: Int = ProcessInfo.processInfo.activeProcessorCount,
        maxConnectionsPerIP: Int = 128
    ) {
        self.hostname = hostname
        self.port = port
        self.workerCount = workerCount
        self.backlog = backlog
        self.maxConnectionsPerIP = maxConnectionsPerIP
        // self.ssl = nil
    }
}

extension EngineServerConfig {
    /// Creates server config for use with Heroku.
    /// Heroku requires that the port be set to $PORT
    /// and the hostname be 0.0.0.0
    public static func heroku() -> EngineServerConfig {
        if let portString = ProcessInfo.processInfo.environment["PORT"],
            let customPort = UInt16(portString) {
            return EngineServerConfig(hostname: "0.0.0.0", port: customPort)
        }
        return EngineServerConfig()
    }
}
Repository: vapor/vapor
File: Sources/Vapor/Engine/Responder.swift (Swift)

Code before:

/// A basic, closure-based responder.
public struct RouteResponder<T>: Responder
    where T: ResponseEncodable
{
    /// Responder closure
    public typealias Closure = (Request) throws -> T

    /// The stored responder closure.
    public let closure: Closure

    /// Create a new basic responder.
    public init(closure: @escaping Closure) {
        self.closure = closure
    }

    /// See: HTTP.Responder.respond
    public func respond(to req: Request) throws -> Future<Response> {
        let encodable = try closure(req)
        return try encodable.encode(for: req)
    }
}

/// A basic, closure-based responder.
public struct ContentRouteResponder<C, T>: Responder
    where C: Content, T: ResponseEncodable
{
    /// Responder closure
    public typealias Closure = (Request, C) throws -> T
    
    /// The stored responder closure.
    public let closure: Closure
    
    /// Create a new basic responder.
    public init(closure: @escaping Closure) {
        self.closure = closure
    }
    
    /// See: HTTP.Responder.respond
    public func respond(to req: Request) throws -> Future<Response> {
        return try req.content.decode(C.self).flatMap(to: Response.self) { content in
            let encodable = try self.closure(req, content)
            return try encodable.encode(for: req)
        }
    }
}

/// MARK: Router

/// Converts a router into a responder.
public struct RouterResponder: Responder {

Review comment (suggestion):
this should be just `RequestDecodable`. no reason to limit to `Content`. 

Code after:

/// A basic, closure-based responder.
public struct RouteResponder<T>: Responder
    where T: ResponseEncodable
{
    /// Responder closure
    public typealias Closure = (Request) throws -> T

    /// The stored responder closure.
    public let closure: Closure

    /// Create a new basic responder.
    public init(closure: @escaping Closure) {
        self.closure = closure
    }

    /// See: HTTP.Responder.respond
    public func respond(to req: Request) throws -> Future<Response> {
        let encodable = try closure(req)
        return try encodable.encode(for: req)
    }
}

/// A basic, closure-based responder.
public struct RequestDecodableResponder<C, T>: Responder
    where C: RequestDecodable, T: ResponseEncodable
{
    /// Responder closure
    public typealias Closure = (Request, C) throws -> T
    
    /// The stored responder closure.
    public let closure: Closure
    
    /// Create a new basic responder.
    public init(closure: @escaping Closure) {
        self.closure = closure
    }
    
    /// See: HTTP.Responder.respond
    public func respond(to req: Request) throws -> Future<Response> {
        return try C.decode(from: req).flatMap(to: Response.self) { content in
            let encodable = try self.closure(req, content)
            return try encodable.encode(for: req)
        }
    }
}

/// MARK: Router

/// Converts a router into a responder.
public struct RouterResponder: Responder {