Remove BlueSocket dependency #39

carlbrown · 2017-08-31T03:03:36Z

I put together a simple (less than 150 line) replacement for BlueSocket for use in Proof of Concepts and Unit Tests that's much easier to understand (since it only needs to do IPv4 TCP server-side bound to all IP addresses on the host). This will let @tanner0101 (and anyone else who needs to) use the repo without needing to mess with any dependencies.

Note that there are features that BlueSocket had that this doesn't support (like listening only to one network interface and leaving other ones alone). I largely consider that a feature. If there are specific things that people need/want, I'll be happy to discuss implementing them in the simpler model.

This PR also enables the ability to change the API for this repo from using (NS)Data to using DispatchData as discussed in #37 - specifically the following part of the conversation:

Part of this is because BlueSocket (that I really want to replace with something much simpler) explicitly reads/writes all socket data in NSData format.

That should be a non-reason. I think very few people care what BlueSocket does. Just drop it and use something else.

Obviously, if we're going to change the HTTP API to use DispatchData, we need to discuss it, so that's not included here. This just makes it possible.

This (hopefully) resolves the root problem with #25 and removes the confusing (but unfortunately expected) errors discussed here: https://lists.swift.org/pipermail/swift-server-dev/Week-of-Mon-20170529/000537.html

call `activate()` to force cleanup fix typo in getsockname on Linux working on Linux don't let broken socket kill the server Split off SimpleServerSocket to its own file pass port along

ianpartridge · 2017-08-31T13:54:58Z

+1 to the big picture, a few comments inline.

carlbrown · 2017-08-31T17:03:16Z

@ianpartridge I don't see comments in line?

ianpartridge · 2017-09-01T12:38:31Z

Er, they vanished. Weird. I'll go through again...

ianpartridge

Comments inline - this also needs running through SwiftLint.

ianpartridge · 2017-09-01T12:39:12Z

Sources/HTTP/SimpleSocket/SimpleServerSocket.swift

+    internal func socketRead(into readBuffer: inout UnsafeMutablePointer<Int8>, maxLength:Int) throws -> Int {
+        //let readBuffer: UnsafeMutablePointer<CChar> = UnsafeMutablePointer<CChar>.allocate(capacity: maxLength)
+        readBuffer.initialize(to: 0x0)
+        let read = recv(self.socketfd, readBuffer, maxLength, Int32(0))


Just return recv(...)

I like putting a breakpoint on the return while troubleshooting and being able to see/print the value. Harder to do if there's no local var (and the optimizer should make them perform equivalently).

You can set a breakpoint then step in/out a few times until recv() returns, but OK.

ianpartridge · 2017-09-01T12:39:34Z

Sources/HTTP/SimpleSocket/SimpleServerSocket.swift

+
+    internal private(set) var isConnected = false
+
+    internal func socketRead(into readBuffer: inout UnsafeMutablePointer<Int8>, maxLength:Int) throws -> Int {


Why not use Data?

See #37 - This makes it possible to measure Data vs DispatchData, which was part of the goal of this exercise.

OK so it's a temporary thing - cool.

ianpartridge · 2017-09-01T12:40:13Z

Sources/HTTP/SimpleSocket/SimpleServerSocket.swift

+        return read
+    }
+
+    @discardableResult internal func socketWrite(from buffer: UnsafeRawPointer, bufSize: Int) throws -> Int {


As this throws it would be better to check the return from send() and throw if <1.

ianpartridge · 2017-09-01T12:40:58Z

Sources/HTTP/SimpleSocket/SimpleServerSocket.swift

+        //print("Shutting down socket \(self.socketfd)")
+        if self.isListening || self.isConnected {
+            //print("Shutting down socket")
+            _ = shutdown(self.socketfd, Int32(SHUT_RDWR))


Is the _ = necessary?

It generates a warning if you ignore the result. It's not marked @discarable in Glibc (or Darwin or both)

Is it safe to discard it then?

Should throw if the return != 0

The reason I had been avoiding throwing if shutdown(2) fails is because it shouldn't be an error if shutdownAndClose gets called on an already-closed-and-invalidated socket. Does that seem reasonable?

ianpartridge · 2017-09-01T12:41:10Z

Sources/HTTP/SimpleSocket/SimpleServerSocket.swift

+    }
+
+    internal func acceptClientConnection() throws -> SimpleServerSocket {
+        let retVal = SimpleServerSocket()


Call this socket?

It's not, though. It only implements a small subset of the socket(2) APIs. Which is all we need for this application, but it's not a general purpose Socket object.

ianpartridge · 2017-09-01T12:44:43Z

Sources/HTTP/SimpleSocket/SimpleSocketConnectionListener.swift

        }

        //In a perfect world, we wouldn't have to clean this all up explicitly,
        // but KDE/heaptrack informs us we're in far from a perfect world

        if !(self.readerSource?.isCancelled ?? true) {
-            self.readerSource?.cancel()
+            #if os(Linux)


This is mysterious... Why all the #if? Needs a comment at least.

I'l comment it. Basically, macOS >= 10.12 needs one implementation, and everything else needs a different one, but I couldn't figure out how to combine the #if os() check with the #available one.

ianpartridge · 2017-09-01T12:45:24Z

Sources/HTTP/SimpleSocket/SimpleSocketConnectionListener.swift

-        } catch {
-            fatalError("Failed to set socket as non-blocking")
-        }
+        socket?.setBlocking(mode: true)


This ignores failure...

Yeah. I'll do another pass with throwing.

ianpartridge · 2017-09-01T12:45:50Z

Sources/HTTP/SimpleSocket/SimpleSocketConnectionListener.swift

-                        let data = Data(bytes: readBuffer.bytes.assumingMemoryBound(to: Int8.self), count: readBuffer.length)
+                if strongSelf.socket?.socketfd ?? -1 > 0 {
+                    let maxLength: Int = Int(strongSelf.readerSource?.data ?? 10000)
+                    var readBuffer: UnsafeMutablePointer<Int8> = UnsafeMutablePointer<Int8>.allocate(capacity: maxLength)


Why aren't we using Data any more?

See conversation with @helje5 in #37

ianpartridge · 2017-09-01T12:47:06Z

Sources/HTTP/SimpleSocket/SimpleSocketSimpleServer.swift

-public class BlueSocketSimpleServer: CurrentConnectionCounting {
-    /// Socket to listen on for connections
-    private let serverSocket: Socket
+public class SimpleSocketSimpleServer: CurrentConnectionCounting {


Too many simples? 🙂

Fair. I'll fix in the next commit

ianpartridge · 2017-09-01T12:47:26Z

Sources/HTTP/SimpleSocket/SimpleSocketSimpleServer.swift


    /// The port we're listening on. Used primarily to query a randomly assigned port during XCTests
    public var port: Int {
        return Int(serverSocket.listeningPort)
    }

    /// Tuning parameter to set the number of queues
-    private var queueMax: Int
+    private var queueMax: Int = 4 //sensible default


Why is this sensible?

Because experimentally (with BlueSocket, so I might need to change it once I run this implementation through load testing), that's the smallest number I could use and still pull a webpage in a browser while a https://github.com/carlbrown/SwiftServerComparison run was in progress.

tanner0101

great work :)

tanner0101 · 2017-09-01T16:35:49Z

Sources/HTTP/SimpleSocket/SimpleServerSocket.swift

+        //print("Shutting down socket \(self.socketfd)")
+        if self.isListening || self.isConnected {
+            //print("Shutting down socket")
+            _ = shutdown(self.socketfd, Int32(SHUT_RDWR))


Should throw if the return != 0

tanner0101 · 2017-09-01T16:37:37Z

Sources/HTTP/SimpleSocket/SimpleServerSocket.swift

+
+    internal func bindAndListen(on port: Int, maxBacklogSize: Int32 = 10000) throws {
+	#if os(Linux)
+        self.socketfd = socket(Int32(AF_INET), Int32(SOCK_STREAM.rawValue), Int32(IPPROTO_TCP))


we set some internal constants in vapor to simplify areas like this https://github.com/vapor/vapor/blob/beta/Sources/TCP/Utilties/Utilities.swift#L28-L34

Oh, wow. I love the touchup of SOCK_STREAM. @ianpartridge, is it okay with you if I mimic https://github.com/vapor/vapor/blob/beta/Sources/TCP/Utilties/Utilities.swift#L32 ?

Sure, anything that makes things more readable is good.

tanner0101 · 2017-09-01T16:39:51Z

Sources/HTTP/SimpleSocket/SimpleServerSocket.swift

+        }
+
+        #if os(Linux)
+            var addr = sockaddr_in(


the code can be a bit cleaner if you use getaddrinfo
https://github.com/vapor/vapor/blob/beta/Sources/TCP/Socket/Socket%2BBind.swift#L7-L40

So with getaddrinfo() you don't have to break out #if OS() there? Is that right? That's handy.

tanner0101 · 2017-09-01T16:41:51Z

Sources/HTTP/SimpleSocket/SimpleSocketConnectionListener.swift


    /// Called by the parser to let us know that a response has started being created
    public func responseBeginning() {
-        self.socketWriterQueue?.async { [weak self] in
+        self.socketWriterQueue.async { [weak self] in


I think all the calls to async here will add quite a bit of overhead. Can we not assume/require that all interaction done w/ the socket be done on the same serial queue?

My concern here is to make sure that we can't get deadlocks. I've had a situation in testing where a read pushed a chunk of data into the pipeline, and it called something that wanted to write, but the write couldn't happen until the read completed, and so it all hung (this was on a future implementation in the context of SSL where OpenSSL's need to keep global state forces a lock around read/write). Maybe I'm just being paranoid. What do you think?

carlbrown · 2017-09-01T18:39:23Z

Ugh @tanner0101 - I pushed a patch to address Ian's comments, and now yours show as outdated and my inline replies aren't appearing above. I wasn't ignoring you. I'm replying both here and inline.

the reason I had been avoiding throwing if shutdown(2) fails is because it shouldn't be an error if shutdownAndClose gets called on an already-closed-and-invalidated socket. Does that seem reasonable?
Oh, wow. I love the touchup of SOCK_STREAM. @ianpartridge, is it okay with you if I mimic https://github.com/vapor/vapor/blob/beta/Sources/TCP/Utilties/Utilities.swift#L32 ?
So with getaddrinfo() you don't have to break out #if OS() there? Is that right?
My concern here is to make sure that we can't get deadlocks. I've had a situation in testing where a read pushed a chunk of data into the pipeline, and it called something that wanted to write, but the write couldn't happen until the read completed, and so it all hung (this was on a future implementation in the context of SSL where OpenSSL's need to keep global state forces a lock around read/write). Maybe I'm just being paranoid. What do you think?

-Carl

seabaylea · 2017-09-03T18:29:37Z

@carlbrown as this touches quite a few files, is this ready to merge as-is with the SOCK_STREAM etc. constants added later? (so it doesn't block changes to the rest of the codebase)

seabaylea · 2017-09-08T10:45:58Z

Ok - I'm going to merge this as-is for now, and we can move further work to another PR.

Remove BlueSocket dependency

747e49d

call `activate()` to force cleanup fix typo in getsockname on Linux working on Linux don't let broken socket kill the server Split off SimpleServerSocket to its own file pass port along

carlbrown requested review from tanner0101, ianpartridge and seabaylea August 31, 2017 03:03

carlbrown mentioned this pull request Aug 31, 2017

no dependencies #25

Closed

4 tasks

This was referenced Aug 31, 2017

Write Callback Block on Queue vs on Write #36

Open

Swift 4 support #40

Closed

ianpartridge reviewed Sep 1, 2017

View reviewed changes

Broken

fab4ecb

tanner0101 reviewed Sep 1, 2017

View reviewed changes

Carl Brown added 2 commits September 1, 2017 13:07

Error handling, comments and a new multi-connection test

1482e09

Get rid of the word Simple showing up so often

cd3f7d7

seabaylea merged commit 6b5b27e into swift-server:develop Sep 8, 2017


		internal private(set) var isConnected = false

		internal func socketRead(into readBuffer: inout UnsafeMutablePointer<Int8>, maxLength:Int) throws -> Int {

Remove BlueSocket dependency #39

Remove BlueSocket dependency #39

Conversation

carlbrown commented Aug 31, 2017 • edited Loading

ianpartridge commented Aug 31, 2017

carlbrown commented Aug 31, 2017

ianpartridge commented Sep 1, 2017

ianpartridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tanner0101 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlbrown commented Sep 1, 2017

seabaylea commented Sep 3, 2017

seabaylea commented Sep 8, 2017

carlbrown commented Aug 31, 2017 •

edited

Loading