Fixed a few minor typos and some force unwraps #259

agnosticdev · 2018-03-29T21:56:21Z

Motivation:

Reviewing the code I found a few typos for readability.
Fixed a few force unwraps.

Modifications:

Minor typos fixes in the following files:
ByteBuffer-core.swift
EventLoopFuture.swwift
Selector.swift
FileRegion.swift
EventLoop.swift
Thread.swift
Bootstrap.swift
CompositeError.swift
LinuxCPUSet.swift
PendingDatagramWritesManager.swift
MarkedCircularBuffer.swift

Fixed force unwraps in the following files:
NIOHTTP1Server/main.swift
NIOChatServer/main.swift
NIOWebSocketServer/main.swift

Result:

The result will be minor typo fixes and a few force unwrap cleanups.

Motivation: Reviewing the code I found a few typos for readability. Fixed a few force unwraps. Modifications: Minor typos fixes in the following files: ByteBuffer-core.swift EventLoopFuture.swwift Selector.swift Codec.swift FileRegion.swift EventLoop.swift Thread.swift Bootstrap.swift CompositeError.swift LinuxCPUSet.swift PendingDatagramWritesManager.swift MarkedCircularBuffer.swift Fixed force unwraps in the following files: NIOHTTP1Server/main.swift NIOChatServer/main.swift NIOWebSocketServer/main.swift Result: The result will be minor typo fixes and a few force unwrap cleanups.

swift-nio-bot · 2018-03-29T21:56:22Z

Can one of the admins verify this patch?

weissi

Thanks very much, minor nits only

weissi · 2018-03-30T09:16:46Z

Sources/NIO/EventLoopFuture.swift

 /// This means that for any `EventLoopFuture` that your code did not create itself (via
 /// `EventLoopPromise.futureResult`), use of `hopTo` is **strongly encouraged** to help guarantee thread-safety. It
-/// should only be elided when thread-safety is provably not needed.
+/// should only be elided when thread-safety is probably not needed.


this fix isn't right, should be 'provably' (as in 'proof')

weissi · 2018-03-30T09:19:20Z

@agnosticdev please also fix your commit message. Its title is 'One line description of your change'

BasThomas · 2018-03-30T10:08:40Z

Sources/NIOHTTP1Server/main.swift

+if let localAddress = channel.localAddress {
+    print("Server started and listening on \(localAddress), htdocs path \(htdocs)")
+} else {
+    print("Server started but the address could not be determined. htdocs path \(htdocs)")


Nit: was ,, now a . in the else case.

BasThomas · 2018-03-30T10:09:24Z

Sources/NIOWebSocketServer/main.swift

 }


 let channel = try! bootstrap.bind(host: "localhost", port: 8888).wait()


Maybe get rid of this force unwrap too?

Motivation: Reviewing the code I found a few typos for readability. Fixed a few force unwraps. Modifications: Minor typos fixes in the following files: ByteBuffer-core.swift EventLoopFuture.swwift Selector.swift Codec.swift FileRegion.swift EventLoop.swift Thread.swift Bootstrap.swift CompositeError.swift LinuxCPUSet.swift PendingDatagramWritesManager.swift MarkedCircularBuffer.swift Fixed force unwraps in the following files: NIOHTTP1Server/main.swift NIOChatServer/main.swift NIOWebSocketServer/main.swift Result: The result will be minor typo fixes and a few force unwrap cleanups.

…into typo_fixes

force unwrap. Motivation: To remove typos. To remove force unwraps, where available. Modifications: Minor typo updates in these files: Sources/NIO/EventLoopFuture.swift Sources/NIOHTTP1Server/main.swift Removed a force unwrap in this file: Sources/NIOWebSocketServer/main.swift Result: The result will be updates for the feedback I received and the removal of a force unwrap in Sources/NIOWebSocketServer/main.swift.

agnosticdev · 2018-03-30T13:02:25Z

@weissi and @BasThomas thank you for the feedback. I have updated my commit to reflect the feedback provided.

weissi · 2018-03-30T14:29:31Z

Sources/NIOWebSocketServer/main.swift

+    }
+
+    // This will never unblock as we don't close the ServerChannel
+    try channel.closeFuture.wait()


@agnosticdev sorry, would you mind taking this line out of the do{}catch{} block just like for the other servers?

@weissi thank you for the feedback, this PR has now been updated.

Motivation: To move NIOWebSocketServer inline with how other servers bind to host. Modifications: The main file in NIOWebSocketServer now accepts command line arguments and binds to a passed in host and port like the other server examples do. Result: The result of my change will bring the NIOWebSocketServer more inline with how the NIOHTTP1 and NIOChat servers examples are setup.

weissi

thanks very much @agnosticdev , that looks good to me.

agnosticdev · 2018-04-02T10:33:52Z

Do I need to sign a CLA to contribute? I noticed that the pull request validation has been outstanding for a 3-4 days now.

normanmaurer · 2018-04-02T10:47:35Z

@swift-nio-bot test this please

weissi · 2018-04-03T10:32:18Z

@swift-nio-bot test this please

Lukasa

Generally looks good, a few minor notes.

Lukasa · 2018-04-03T10:39:51Z

Sources/NIO/Codec.swift

    public func channelRead(ctx: ChannelHandlerContext, data: NIOAny) {
        var buffer = self.unwrapInboundIn(data)
-
+        


Mind removing this whitespace?

Lukasa · 2018-04-03T10:41:02Z

Sources/NIO/EventLoopFuture.swift

 /// This means that for any `EventLoopFuture` that your code did not create itself (via
 /// `EventLoopPromise.futureResult`), use of `hopTo` is **strongly encouraged** to help guarantee thread-safety. It
-/// should only be elided when thread-safety is provably not needed.
+/// should only be elided when thread-safety is proved to be not needed.


This doesn't need a change: Merriam Webster defines "provably" as the adjectival form of "prove".

Lukasa · 2018-04-03T10:41:51Z

Sources/NIOChatServer/main.swift

+    print("ChatServer started and listening on \(localAddress)")
+} else {
+    print("ChatServer started but the address could not be determined.")
+}


I don't think we need to change these: we will always know our local address at this stage.

@Lukasa I was unsure about that one. Sure, this can't actually happen but then always having a always having a force unwrap here is also not great in the examples... So I thought that is okay and I thought @normanmaurer approved too but he actually didn't, he just ran the tests. Apologies!

That was my motivation in making this change. I wanted to remove the force unwraps in the examples because I know that avoiding force unwraps is a common Swift development technique.

IMO, blindly replacing force unwraps with conditional binding is no better than blindly force unwrapping values. A force unwrap should only be used when the value being optional would represent programmer error, or when it is literally not possible to recover from the optional case.

Put another way, this is a perfectly good force-unwrap that (I hope) no-one would suggest we should replace with guard let:

let x = URL(string: "https://github.com/")!

A more complex example that I consider to be the same is this code block:

swift-nio/Sources/NIOTLS/SNIHandler.swift

Lines 197 to 228 in 2839ef0

private func parseRecordHeader(buffer: inout ByteBuffer) throws -> Int {

// First, the obvious check: are there enough bytes for us to parse a complete

// header? Because of this check we can use unsafe integer reads in the rest of

// this function, as we'll only ever read tlsRecordHeaderLength number of bytes

// here.

guard buffer.readableBytes >= tlsRecordHeaderLength else {

throw InternalSniErrors.recordIncomplete

}

// Check the content type.

let contentType: UInt8 = buffer.readInteger()!

guard contentType == tlsContentTypeHandshake else {

// Whatever this is, it's not a handshake message, so something has gone

// wrong. We're going to fall back to the default handler here and let

// that handler try to clean up this mess.

throw InternalSniErrors.invalidRecord

}

// Now, check the major version.

let majorVersion: UInt8 = buffer.readInteger()!

guard majorVersion == 3 else {

// A major version of 3 is the major version used for SSLv3 and all subsequent versions

// of the protocol. If that's not what this is, we don't know what's happening here.

// Again, let the default handler make sense of this.

throw InternalSniErrors.invalidRecord

}

// Skip the minor version byte, then grab the content length.

buffer.moveReaderIndex(forwardBy: 1)

let contentLength: UInt16 = buffer.readInteger()!

return Int(contentLength)

}

This block contains three force-unwraps, but all of them are "safe" because of the guard at the top of the function. If any of those force-unwraps ever fails, that failure will be because I have made an error in assuming that tlsRecordHeaderLength is at least 5, or because of an error in the ByteBuffer implementation. Throwing an error in that case would be entirely misleading. This is exactly like force-unwrapping the URL above: the force unwrap is justified by my other code, and I simply cannot express to the compiler that I know this will always succeed in any way other than a force unwrap.

Returning to localAddress: my argument is that NIO makes a promise that localAddress will always be well-defined and non-optional once the bind promise returns, until the file descriptor is closed. If that promise is not kept, NIO is quite severely wrong about the state of the world, and it should probably crash rather than keep running in this confused state. For this reason, I consider the force-unwrap here to be totally safe. It's a short way to say "this value being nil here indicates a malfunctioning program".

@Lukasa agreed if you can know that it can never be nil and that is the case for the code you quote.
But we're talking about the example code here and users don't know this can not be nil as they're not NIO devs.
I don't really have a good suggestion because we can't change the API (as not all channels have localAddresses) but we also shouldn't make everybody force unwrap here I don't think. Maybe the solution is to have a String returning function that is non-optional and just fills in "<not available>" or something if unavailable.

I mean, isn't the solution to this to write a comment that explains the force unwrap?

Let me reframe that. Right now the patch gives an equally misleading indication about the way this API behaves, because it implies that sometimes NIO may just be unable to find the local address for no good reason ("address could not be determined"). That's not right: there are well defined cases when the address can be found, and well defined cases where it cannot. I can enumerate them right now:

getsockname has no return value, which means that there is no bound address on the machine at this time, meaning either the socket is closed or was never bound.

SocketAddress cannot parse the return value of getsockname, which means this is not an address family that NIO natively understands.

A coding error where NIO has dropped the ball somehow, and a valid SocketAddress-representable socket address was not correctly parsed or returned by the OS.

If the goal of this is to educate users about our APIs, we should do that here by placing a comment that says, basically: "We know the local address can be represented in SocketAddress because we just did so earlier, and we know that NIO promises that if it can represent the local address in a SocketAddress then it will definitely return it from localAddress after bind but before we call close. As a result we can safely force unwrap this result here, as a nil value represents a bug either in this code or NIO."

I mean, if we really hate the force-unwrap, we can do guard let ... else { fatalError() }, but that's just a long way of spelling !.

@Lukasa everything you write is totally accurate, yet often the user may just want to print a string and I think many people just don't like to force unwrap for that...
And sure, a better comment would probably help here to give the users confidence on when then can force unwrap this 'safely'.

I don't really know what to do here.

@Lukasa and @weissi Thank you for much for the insightful feedback. After reading through the explanation on specific cases where the local address could not be found, I agree that the, "address could not be determined," message is misleading. I also agree that a force unwrap failure here signifies a much larger issue in NIO. I do however feel that more accurate information on why the force unwrap failed would be beneficial as well. As a middle ground between using a force unwrap and the current patch in place I think the best option would be to use a guard statement to unwrap localAddress. If a failure should ever occur I an error message provides more specific information on what could be happening in these specific failures.

The new PR addressing this feedback is up here: #273
Thank you very much for the time and the explanation provided.

Lukasa · 2018-04-03T10:41:54Z

Sources/NIOHTTP1Server/main.swift

+    print("Server started and listening on \(localAddress), htdocs path \(htdocs)")
+} else {
+    print("Server started but the address could not be determined, htdocs path \(htdocs)")
+}


I don't think we need to change these: we will always know our local address at this stage.

Lukasa · 2018-04-03T10:42:09Z

Sources/NIOWebSocketServer/main.swift

+    print("Server started and listening on \(localAddress)")
+} else {
+    print("Server started but the address could not be determined.")
+}


I don't think we need to change these: we will always know our local address at this stage.

weissi · 2018-04-03T10:45:08Z

thanks very much @agnosticdev , merged. And no, we don't require a CLA but we had a 4 day weekend here in Europe so not much happened :)

agnosticdev · 2018-04-03T10:54:18Z

Wanted to check in; I see that there was outstanding comments before the PR was merged. Do you want me to address any of this feedback?

weissi · 2018-04-03T11:32:58Z

@agnosticdev / @Lukasa sorry! @Lukasa and I hit a race condition. @agnosticdev would you mind making a new PR addressing these issues?

agnosticdev changed the title ~~One line description of your change~~ Fixed a few minor typos and some force unwraps Mar 30, 2018

weissi requested changes Mar 30, 2018

View reviewed changes

BasThomas reviewed Mar 30, 2018

View reviewed changes

agnosticdev added 3 commits March 30, 2018 07:38

Merge branch 'typo_fixes' of https://github.com/agnosticdev/swift-nio …

aed77a6

…into typo_fixes

weissi reviewed Mar 30, 2018

View reviewed changes

weissi added the 🔨 semver/patch No public API change. label Mar 30, 2018

weissi approved these changes Mar 30, 2018

View reviewed changes

Merge branch 'master' into typo_fixes

9bbd2a5

Lukasa requested changes Apr 3, 2018

View reviewed changes

weissi merged commit 66e4fdf into apple:master Apr 3, 2018

Lukasa added this to the 1.4.0 milestone Apr 9, 2018

		}


		let channel = try! bootstrap.bind(host: "localhost", port: 8888).wait()

		public func channelRead(ctx: ChannelHandlerContext, data: NIOAny) {
		var buffer = self.unwrapInboundIn(data)

	private func parseRecordHeader(buffer: inout ByteBuffer) throws -> Int {
	// First, the obvious check: are there enough bytes for us to parse a complete
	// header? Because of this check we can use unsafe integer reads in the rest of
	// this function, as we'll only ever read tlsRecordHeaderLength number of bytes
	// here.
	guard buffer.readableBytes >= tlsRecordHeaderLength else {
	throw InternalSniErrors.recordIncomplete
	}

	// Check the content type.
	let contentType: UInt8 = buffer.readInteger()!
	guard contentType == tlsContentTypeHandshake else {
	// Whatever this is, it's not a handshake message, so something has gone
	// wrong. We're going to fall back to the default handler here and let
	// that handler try to clean up this mess.
	throw InternalSniErrors.invalidRecord
	}

	// Now, check the major version.
	let majorVersion: UInt8 = buffer.readInteger()!
	guard majorVersion == 3 else {
	// A major version of 3 is the major version used for SSLv3 and all subsequent versions
	// of the protocol. If that's not what this is, we don't know what's happening here.
	// Again, let the default handler make sense of this.
	throw InternalSniErrors.invalidRecord
	}

	// Skip the minor version byte, then grab the content length.
	buffer.moveReaderIndex(forwardBy: 1)
	let contentLength: UInt16 = buffer.readInteger()!
	return Int(contentLength)
	}

Fixed a few minor typos and some force unwraps #259

Fixed a few minor typos and some force unwraps #259

Uh oh!

Conversation

agnosticdev commented Mar 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swift-nio-bot commented Mar 29, 2018

Uh oh!

weissi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weissi commented Mar 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agnosticdev commented Mar 30, 2018

Uh oh!

weissi Mar 30, 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 left a comment

Choose a reason for hiding this comment

Uh oh!

agnosticdev commented Apr 2, 2018

Uh oh!

normanmaurer commented Apr 2, 2018

Uh oh!

weissi commented Apr 3, 2018

Uh oh!

Lukasa left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weissi commented Apr 3, 2018

Uh oh!

agnosticdev commented Apr 3, 2018

Uh oh!

weissi commented Apr 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

agnosticdev commented Mar 29, 2018 •

edited

Loading

weissi Mar 30, 2018 •

edited

Loading