Throw SocketAddressError (cf. fatalError) in SocketAddress.convert for unknown address family #2477
Conversation
…r unknown address family Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
| let address: SocketAddress = self.sockaddrVector[i].convert() | ||
| let address: SocketAddress | ||
| do { address = try self.sockaddrVector[i].convert() } | ||
| catch { fatalError("Socket address conversion failed.") } |
There was a problem hiding this comment.
Good to know—I assumed that try! would be discouraged in review. Although I appreciate that all we do here is fail with a different string.
Changed in 805f1ca.
There was a problem hiding this comment.
try! falls into the same bucket as !, which is that you need to write down why you know it won't crash in a comment.
| let address: SocketAddress = self.sockaddrVector[i].convert() | ||
| let address: SocketAddress | ||
| do { address = try self.sockaddrVector[i].convert() } | ||
| catch { fatalError("Socket address conversion failed.") } |
There was a problem hiding this comment.
More broadly, should we tolerate this error in this location?
There was a problem hiding this comment.
That's a great question.
I suppose we have to choose between failing hard or silently dropping these messages, given that we will be unable to construct an AddressedEnvelope without a SocketAddress.
I opted to fail because:
- It's closest to the behaviour before this patch—where it would have still crashed, just inside
convert(). - Above these lines there are a number of
preconditionchecks so failing hard seemed to be "the done thing" here.
Happy to revisit though if you think this is the wrong choice here.
There was a problem hiding this comment.
I think there's a third option, which is to throw. The preconditions above are correctness checks, but this is not. Throwing here is consistent with the scalar read path.
Sources/NIOPosix/SocketChannel.swift
Outdated
| @@ -651,7 +651,7 @@ final class DatagramChannel: BaseSocketChannel<Socket> { | |||
| metadata = nil | |||
| } | |||
|
|
|||
| let msg = AddressedEnvelope(remoteAddress: rawAddress.convert(), | |||
| let msg = AddressedEnvelope(remoteAddress: try rawAddress.convert(), | |||
There was a problem hiding this comment.
Can we lift the call to convert higher in the .processed block? Let's not trigger any side-effects until we know we're going to process the read.
There was a problem hiding this comment.
Done in 48d98b8.
Signed-off-by: Si Beaumont <beaumont@apple.com>
Signed-off-by: Si Beaumont <beaumont@apple.com>
simonjbeaumont
force-pushed
the
sb-throw-non-fatal-error-for-unknown-sockets
branch
from
f7ca742 to
48d98b8
Compare
Signed-off-by: Si Beaumont <beaumont@apple.com>
|
@swift-server-bot test this please |
Signed-off-by: Si Beaumont <beaumont@apple.com>
|
5.9 allocation regression is known and to be investigated |
Motivation:
NIO currently fails with
fatalErrorwhen converting asockaddr_storageif it is not one of the socket addresses that NIO explicitly supports (AF_UNIX,AF_INET, orAF_INET6). However, since NIO offers API that allows users to create sockets out of band (withConnectedSocket(_:)andwithBoundSocket(_:)), it's possible a user has provided a socket of a different family.Modifications:
Instead of crashing,
SocketAddress.convert()now throwsSocketAddressError.unsupported, and whether to crash (or just propagate the error) is moved up the stack.Result:
It's possible to bootstrap clients and servers with other kinds of sockets, even if NIO cannot convert them to one of the
SocketAddresscases it knows about.