propagate socket errors on .reset #325

weissi · 2018-04-17T18:12:00Z

Motivation:

Since #286, we do eagerly detect connection resets which is good. To
resolve the situation we first try to read everything off the socket. If
something fails there, everything is good and the client gets the
correct error. If that however doesn't lead to any errors, we would
close the connection with error: ChannelError.eof which the client
would then see for example on a connect that happened to get
ECONNRESET or ECONNREFUSED. That's not right so these situations are
fixed here.

Modifications:

on reset, try to get the socket error if any
if no socket error was detected, we send a ECONNRESET because we
know we have been reset but not why and didn't encounter any errors
write tests for these situations

Result:

connection resets that don't hit any other errors will now see the
correct error

normanmaurer · 2018-04-17T18:16:20Z

Sources/NIO/BaseSocketChannel.swift

-        self.close0(error: ChannelError.eof, mode: .all, promise: nil)
+
+        if self.lifecycleManager.isRegistered {
+            // if the socket is still open, let's try to get the actual socket error from the socket


@weissi hmm this comment is confusing as we not really check for open but for registered above. Can you clarify ?

good point. We close the socket when unregistering it but I should probably do something like this:

if self.socket.isOpen { assert(self.lifecycleManager.isRegistered) ...

Lukasa · 2018-04-17T18:25:05Z

Sources/NIO/BaseSocketChannel.swift

+
+        if self.lifecycleManager.isRegistered {
+            // if the socket is still open, let's try to get the actual socket error from the socket
+            let result: Int32


Does this need to be declared outside the do? It isn't used elsewhere.

Lukasa · 2018-04-17T18:31:30Z

Sources/NIO/BaseSocketChannel.swift

+                }
+            } catch {
+                self.close0(error: error, mode: .all, promise: nil)
+            }


This code could be cleaned up, I think, either by stack allocating an existential Error that each branch sets, and then calls close0 with it, or by having the top two branches stack allocate a concrete IOError.

Either way we should aim to have fewer of these calls to close0.

reducing to 2 as we chatted about

Lukasa · 2018-04-17T18:31:43Z

Sources/NIO/BaseSocketChannel.swift

+        if self.socket.isOpen {
+            assert(self.lifecycleManager.isRegistered)
+            // if the socket is still open, let's try to get the actual socket error from the socket
+            let result: Int32


This doesn't need to be declared outside the block, it's not used outside it.

Lukasa · 2018-04-17T18:34:06Z

Tests/NIOTests/ChannelTests.swift

+            })
+            return sock
+        }
+        let serverSockAddress = serverSockSyncQ.sync { try! serverSock.localAddress() }


These two synchronous queue operations appear to be unnecessary.

Lukasa · 2018-04-17T18:38:45Z

Tests/NIOTests/ChannelTests.swift

+        let g = DispatchGroup()
+        let q = DispatchQueue(label: "q")
+        do {
+            // spawn 64 connects, likely some will already get ECONNREFUSED as the backlog isn't large enough


Nit: think you're looking for ECONNRESET here.

Lukasa · 2018-04-17T18:39:09Z

Tests/NIOTests/ChannelTests.swift

+                                var pfd = pollfd(fd: fd, events: Int16(POLLIN | POLLERR | POLLHUP), revents: 0)
+                                // wait a maximum of 100ms to accept a new client, just to be guaranteed we don't get stuck here.
+                                if poll(&pfd, 1, 100) == 1 {
+                                    return accept(fd, nil, nil)


Any reason not to just block in accept here?

yeah, that would hang forever so we need the 100ms timeout here

In what circumstance will it hang forever?

@Lukasa well, it calls accept on an open socket. That would only ever return if there was someone connecting. But if there are no further connections (remember the 64 client connections each might either fail or (at least partially) succeed)

Lukasa · 2018-04-17T18:39:49Z

Tests/NIOTests/ChannelTests.swift

+                        if let accepted = accepted {
+                            // set SO_LINGER to 0, which will cause most TCP stacks to send an RST packet
+                            var lin = linger(l_onoff: 1, l_linger: 0)
+                            setsockopt(accepted, SOL_SOCKET, SO_LINGER, &lin, socklen_t(MemoryLayout.size(ofValue: lin)))


If SO_LINGER can get a RST packet, why do we need 64 connections?

@Lukasa the SO_LINGER works well for Darwin, the 64 connections works well for Linux ;)

Ah, can you update the comments to make that clear?

weissi · 2018-04-18T13:10:43Z

@Lukasa feedback addressed

Lukasa · 2018-04-18T13:32:36Z

Sources/NIO/BaseSocketChannel.swift

+                if result != 0 {
+                    // we have a socket error, let's forward
+                    // this path will be executed on Linux (EPOLLERR) & Darwin (ev.fflags != 0)
+                    error = IOError(errnoCode: result, reason: "connection reset (error set)")


Rather than passing reason can you pass function? That'll give us better debug strings here.

@Lukasa there's no function that errors here, right?

Just make one up. ;) The key advantage is that we get the textual description of the underlying errno, rather than your hard-coded guess.

that's not how it's intended. The reason is there so it gives you a better message. I acknowledge that we had a bug here where that was worse. In case where you don't want to provide a reason as it would be "connect failed, errno \(errno)", you can use function: #function as that's shorter.

Lukasa · 2018-04-18T13:37:17Z

Tests/NIOTests/ChannelTests.swift

+                        break
+                    } catch {
+                        XCTFail("unexpected error \(error)")
+                    }


Here's my question: is there any reason we can't do this with a regular server socket channel? Create it, set the child channel options, turn off auto-read so that we only accept a maximum of one.

initially I just closed that socket from elsewhere so I needed to tolerate EBADF. I have now changed that so possibly I could go back to a ServerSocketChannel

@Lukasa I actually will scrap this whole test. It just isn't causing the ECONNRESET reliably enough on all the platforms and there's always room for random failure of the test (unless I don't assert that at least one actually gets ECONNRESET). But tbh I think pretty much everything is covered by the ECONNREFUSED test already...

Lukasa · 2018-04-18T13:37:47Z

Tests/NIOTests/ChannelTests.swift

+        // we deliberately don't set SO_REUSEADDR
+        XCTAssertNoThrow(try serverSock.bind(to: SocketAddress(ipAddress: "127.0.0.1", port: 0)))
+        XCTAssertNoThrow(try serverSock.withUnsafeFileDescriptor { fd in
+            try Posix.listen(descriptor: fd, backlog: 1)


Why are you calling listen?

true, actually don't need that

Lukasa · 2018-04-18T17:25:57Z

Tests/NIOTests/ChannelTests.swift

+        let serverSockAddress = try! serverSock.localAddress()
+        XCTAssertNoThrow(try serverSock.close())
+
+        for _ in 0..<64 {


Do we need the loop? We fail in all cases other than the throw, which suggests we always throw.

@Lukasa in my tests it made the probability that we also hit the async connect path a lot higher (as in pretty much always). Therefore I thought that's a good idea :)

Lukasa

LGTM.

Motivation: Since apple#286, we do eagerly detect connection resets which is good. To resolve the situation we first try to read everything off the socket. If something fails there, everything is good and the client gets the correct error. If that however doesn't lead to any errors, we would close the connection with `error: ChannelError.eof` which the client would then see for example on a connect that happened to get `ECONNRESET` or `ECONNREFUSED`. That's not right so these situations are fixed here. Modifications: - on reset, try to get the socket error if any - if no socket error was detected, we send a `ECONNRESET` because we know we have been reset but not why and didn't encounter any errors - write tests for these situations Result: connection resets that don't hit any other errors will now see the correct error

weissi requested a review from Lukasa April 17, 2018 18:12

normanmaurer requested changes Apr 17, 2018

View reviewed changes

weissi force-pushed the jw-propagate-sock-errors branch from ff4e63b to 1e960ff Compare April 17, 2018 18:29

Lukasa requested changes Apr 17, 2018

View reviewed changes

weissi force-pushed the jw-propagate-sock-errors branch from 1e960ff to c88dbf4 Compare April 18, 2018 13:05

Lukasa requested changes Apr 18, 2018

View reviewed changes

weissi force-pushed the jw-propagate-sock-errors branch 3 times, most recently from adf7d72 to 0b91305 Compare April 18, 2018 17:22

Lukasa requested changes Apr 18, 2018

View reviewed changes

weissi force-pushed the jw-propagate-sock-errors branch from 0b91305 to 640cf11 Compare April 18, 2018 17:30

Lukasa approved these changes Apr 18, 2018

View reviewed changes

normanmaurer approved these changes Apr 18, 2018

View reviewed changes

weissi force-pushed the jw-propagate-sock-errors branch from 640cf11 to 7a048cc Compare April 18, 2018 17:37

weissi merged commit a1ab3f3 into apple:master Apr 18, 2018

weissi deleted the jw-propagate-sock-errors branch April 18, 2018 17:56

Lukasa added the 🔨 semver/patch No public API change. label Apr 19, 2018

Lukasa added this to the 1.5.0 milestone Apr 19, 2018

propagate socket errors on .reset #325

propagate socket errors on .reset #325

Uh oh!

Conversation

weissi commented Apr 17, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weissi Apr 18, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

weissi Apr 17, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

weissi commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Lukasa left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

weissi Apr 18, 2018 •

edited

Loading

weissi Apr 17, 2018 •

edited

Loading