Support UNIX Domain Sockets by krzyzanowskim · Pull Request #151 · swift-server/async-http-client

krzyzanowskim · 2020-01-19T00:18:03Z

Adds support for UNIX Domain Socket requests.

Usage:

let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
let socketURL = URL(string: "unix:///var/run/docker.sock")!
let req = try HTTPClient.Request(url: URL(string: "/users/list", relativeTo: socketURL)!, method: .GET)
let response = try httpClient.execute(request: req).wait()

swift-server-bot · 2020-01-19T00:18:05Z

Can one of the admins verify this patch?

swift-server-bot · 2020-01-19T00:18:05Z

Can one of the admins verify this patch?

swift-server-bot · 2020-01-19T00:18:05Z

Can one of the admins verify this patch?

Lukasa

Thanks for this patch!

I don't think we should use file:// as the scheme for Unix domain sockets: file:// is a very well-understood scheme from the perspective of browser user agents, and overloading that semantic here for another purpose seems unwise. I think the traditional choice for a scheme for Unix sockets is unix://, which works better. We could also choose to use unix+http://, but I think I'd begin with unix:// for now: we can always widen the list.

Lukasa · 2020-01-20T09:32:17Z

Sources/AsyncHTTPClient/HTTPHandler.swift

        }

        var redirectState: RedirectState?
+        private var value: HTTPRequest


I don't love using a protocol existential here. I think the splitting of basic request types is a good idea, but I'd rather use an enumeration that we can switch over than the protocol existential.

It seems the only difference between implementations is how they handle scheme? Why not just add a unix:// to a list of supported schemes?

Why not just add a unix:// to a list of supported schemes?

slightly different asserts in Request constructor

I'd rather use an enumeration that we can switch over than the protocol existential.

I'm not a fan. Do you think about if'ing in every Request property?

I've rework it with enum: d3bbf37 let me know if that's something you love more @Lukasa 😅

artemredkin · 2020-01-20T17:10:20Z

Two questions:

What is the use-case you have in mind? Never personally encountered it, just curious
Cory, could ssl negotiation be run on top of the unix socket? Is there anything tcp/ip specific? Is it something potentially useful?

krzyzanowskim · 2020-01-20T17:11:44Z

What is the use-case you have in mind?

My use case is local Docker server over unix socket

artemredkin · 2020-01-20T17:13:42Z

Point 2 would probably not be useful since we running it on the same machine...

krzyzanowskim · 2020-01-20T17:27:58Z

could ssl negotiation be run on top of the unix socket?

in general yes. I don't know if nio can handle it though. Looks like eg. postgres can utilise that

Lukasa · 2020-01-20T17:41:14Z

Cory, could ssl negotiation be run on top of the unix socket? Is there anything tcp/ip specific? Is it something potentially useful?

You can run TLS over the unix socket, it's mostly not useful though it can be if the process on the other side of the unix socket is a TCP proxy. It's worth keeping it as a thing we could potentially support in future.

in general yes. I don't know if nio can handle it though.

NIO's TLS implementation is 100% composable, so it can be run over literally any stream transport. 😉

…http request.

Lukasa · 2020-01-27T10:14:11Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+                self.host = ""
+            } else {
+                throw HTTPClientError.unsupportedScheme(scheme)
            }


This block can be rewritten to be substantially cleaner by defining some helpers:

extension Kind { private static var hostSchemes = ["http", "https"] private static var unixSchemes = ["unix"] init(forScheme scheme: String) throws { if Kind.hostSchemes.contains(scheme) { self = .host } else if Kind.unixSchemes.contains(scheme) { self = .unixSocket } else { throw HTTPClientError.unsupportedScheme } } func hostFromURL(_ url: URL) throws -> String { switch self { case .host: guard let host = url.host else { throw HTTPClientError.emptyHost } return host case .unixSocket: return "" } } }

This code then becomes:

self.kind = try Kind(scheme: scheme) self.host = try self.kind.hostFromURL(url)

We can then also rewrite kind.isSchemeSupported in terms of our statics. This should put most of the complexity into the enum definition, which helps keep the code elsewhere a lot nicer.

thanks for the feedback. Applied as requested.

Lukasa

Cool, I think I'm happy with this. @weissi what do you think?

weissi

Thanks very much. Looks good to me

weissi · 2020-01-27T13:37:19Z

@swift-server-bot test this please

Sources/AsyncHTTPClient/HTTPHandler.swift

artemredkin

One question, otherwise LG

weissi · 2020-01-27T13:43:11Z

sorry @krzyzanowskim , this repo is set up with an formating requirements :|

=> Checking format... formatting issues!
diff --git a/Sources/AsyncHTTPClient/HTTPClient.swift b/Sources/AsyncHTTPClient/HTTPClient.swift
index bb93908..dd50433 100644
--- a/Sources/AsyncHTTPClient/HTTPClient.swift
+++ b/Sources/AsyncHTTPClient/HTTPClient.swift
@@ -290,12 +290,12 @@ public class HTTPClient {
         }
 
         eventLoopChannel.map { channel in
-              task.setChannel(channel)
-            }
-            .flatMap { channel in
-                channel.writeAndFlush(request)
-            }
-            .cascadeFailure(to: task.promise)
+            task.setChannel(channel)
+        }
+        .flatMap { channel in
+            channel.writeAndFlush(request)
+        }
+        .cascadeFailure(to: task.promise)
 
         return task
     }
diff --git a/Sources/AsyncHTTPClient/HTTPHandler.swift b/Sources/AsyncHTTPClient/HTTPHandler.swift
index eb86ee2..fc83eac 100644
--- a/Sources/AsyncHTTPClient/HTTPHandler.swift
+++ b/Sources/AsyncHTTPClient/HTTPHandler.swift
@@ -88,7 +88,6 @@ extension HTTPClient {
 
     /// Represent HTTP request.
     public struct Request {
-
         /// Represent kind of Request
         enum Kind {
             /// Remote host request.
@@ -111,13 +110,13 @@ extension HTTPClient {
 
             func hostFromURL(_ url: URL) throws -> String {
                 switch self {
-                    case .host:
-                        guard let host = url.host else {
-                            throw HTTPClientError.emptyHost
-                        }
-                        return host
-                    case .unixSocket:
-                        return ""
+                case .host:
+                    guard let host = url.host else {
+                        throw HTTPClientError.emptyHost
+                    }
+                    return host
+                case .unixSocket:
+                    return ""
                 }
             }
 
@@ -844,7 +843,7 @@ internal struct RedirectHandler<ResponseType> {
             return nil
         }
 
-        guard request.kind.supports(scheme: self.request.scheme) else {
+        guard self.request.kind.supports(scheme: self.request.scheme) else {
             return nil

mind running SwiftFormat on it?

lou-lan · 2020-01-27T13:48:19Z

I am test it, got some error:

Fatal error: Error raised at top level: HTTPClientError.unsupportedScheme("file"): file /BuildRoot/Library/Caches/com.apple.xbs/Sources/swiftlang/swiftlang-1100.8.280/swift/stdlib/public/core/ErrorType.swift, line 200
2020-01-27 21:46:57.444788+0800 Demo[30365:399706] Fatal error: Error raised at top level: HTTPClientError.unsupportedScheme("file"): file /BuildRoot/Library/Caches/com.apple.xbs/Sources/swiftlang/swiftlang-1100.8.280/swift/stdlib/public/core/ErrorType.swift, line 200
(lldb)

import Foundation
import AsyncHTTPClient

let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
let socketURL = URL(fileURLWithPath: "/var/run/docker.sock")
let req = try HTTPClient.Request(url: URL(string: "/_ping", relativeTo: socketURL)!, method: .GET)
let response = try httpClient.execute(request: req).wait()
print(response)

OS: macOS
Swift version 5.1.3

weissi · 2020-01-27T13:48:26Z

@swift-server-bot add to whitelist

weissi · 2020-01-27T13:49:13Z

@lou-lan try unix:///path/to/file instead of file:///...

lou-lan · 2020-01-27T14:05:18Z

@weissi I got the same error message：

Fatal error: Error raised at top level: HTTPClientError.unsupportedScheme("file"): file /BuildRoot/Library/Caches/com.apple.xbs/Sources/swiftlang/swiftlang-1100.8.280/swift/stdlib/public/core/ErrorType.swift, line 200
2020-01-27 22:01:34.073115+0800 Demo[31194:417905] Fatal error: Error raised at top level: HTTPClientError.unsupportedScheme("file"): file /BuildRoot/Library/Caches/com.apple.xbs/Sources/swiftlang/swiftlang-1100.8.280/swift/stdlib/public/core/ErrorType.swift, line 200

import Foundation
import AsyncHTTPClient

let httpClient = HTTPClient(eventLoopGroupProvider: .createNew)
let socketURL = URL(fileURLWithPath: "unix:///var/run/docker.sock")
let req = try HTTPClient.Request(url: URL(string: "/_version", relativeTo: socketURL)!, method: .GET)
let response = try httpClient.execute(request: req).wait()
print(response)

krzyzanowskim · 2020-01-27T14:06:40Z

@lou-lan I updated example in description. Don't use fileURLWithPath, it overrides your URL schema

lou-lan · 2020-01-27T14:09:38Z

@krzyzanowskim Success, thank you very much, very cool PR.

Support UNIX Domain Socket url

4932cb5

Use relative URL as socket url

41ee04e

Lukasa requested changes Jan 20, 2020

View reviewed changes

Adapt unix scheme for sockets

ac69f69

Use Request.Kind to distinguish between host and unix socket kind of …

d3bbf37

…http request.

Lukasa reviewed Jan 27, 2020

View reviewed changes

refactor: Move some request logic to enum helpers

bbd47f7

Lukasa approved these changes Jan 27, 2020

View reviewed changes

krzyzanowskim marked this pull request as ready for review January 27, 2020 13:27

weissi approved these changes Jan 27, 2020

View reviewed changes

Merge branch 'master' into marcin/unix-socket-request

cb19a2d

weissi requested a review from artemredkin January 27, 2020 13:37

artemredkin reviewed Jan 27, 2020

View reviewed changes

Sources/AsyncHTTPClient/HTTPHandler.swift Outdated Show resolved Hide resolved

artemredkin requested changes Jan 27, 2020

View reviewed changes

krzyzanowskim added 2 commits January 27, 2020 14:47

Update formatting

45c443f

Revert host to let

535303b

weissi requested a review from artemredkin January 27, 2020 13:48

artemredkin approved these changes Jan 27, 2020

View reviewed changes

krzyzanowskim added 2 commits January 27, 2020 14:53

Don't use Self to please Swift 5.0

5233d5c

Ups. Wrong property updated previously

b6cf0a4

artemredkin added the 🆕 semver/minor Adds new public API. label Jan 27, 2020

artemredkin added this to the 1.1.0 milestone Jan 27, 2020

weissi merged commit e90f5fd into swift-server:master Jan 27, 2020

PopFlamingo mentioned this pull request Feb 2, 2020

Add a connection pool #105

Merged

8 tasks

Conversation

krzyzanowskim commented Jan 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-server-bot commented Jan 19, 2020

Uh oh!

swift-server-bot commented Jan 19, 2020

Uh oh!

swift-server-bot commented Jan 19, 2020

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Lukasa Jan 20, 2020

Choose a reason for hiding this comment

Uh oh!

artemredkin Jan 20, 2020

Choose a reason for hiding this comment

Uh oh!

krzyzanowskim Jan 20, 2020

Choose a reason for hiding this comment

Uh oh!

krzyzanowskim Jan 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artemredkin commented Jan 20, 2020

Uh oh!

krzyzanowskim commented Jan 20, 2020

Uh oh!

artemredkin commented Jan 20, 2020

Uh oh!

krzyzanowskim commented Jan 20, 2020

Uh oh!

Lukasa commented Jan 20, 2020

Uh oh!

Lukasa Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

krzyzanowskim Jan 27, 2020

Choose a reason for hiding this comment

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

weissi commented Jan 27, 2020

Uh oh!

Uh oh!

artemredkin left a comment

Choose a reason for hiding this comment

Uh oh!

weissi commented Jan 27, 2020

Uh oh!

lou-lan commented Jan 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weissi commented Jan 27, 2020

Uh oh!

weissi commented Jan 27, 2020

Uh oh!

lou-lan commented Jan 27, 2020

Uh oh!

krzyzanowskim commented Jan 27, 2020

Uh oh!

lou-lan commented Jan 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

krzyzanowskim commented Jan 19, 2020 •

edited

Loading

krzyzanowskim Jan 26, 2020 •

edited

Loading

lou-lan commented Jan 27, 2020 •

edited

Loading