-
Notifications
You must be signed in to change notification settings - Fork 719
Fixed a few minor typos and some force unwraps #259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
4114588
339d7b5
aed77a6
ca9528d
2dd9785
9bbd2a5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -322,7 +322,7 @@ public struct EventLoopPromise<T> { | |
| /// | ||
| /// 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. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't need a change: Merriam Webster defines "provably" as the adjectival form of "prove". |
||
| /// | ||
| /// The "thread affinity" of `EventLoopFuture`s is critical to writing safe, performant concurrent code without | ||
| /// boilerplate. It allows you to avoid needing to write or use locks in your own code, instead using the natural | ||
|
|
@@ -734,7 +734,7 @@ extension EventLoopFuture { | |
| /// Fulfill the given `EventLoopPromise` with the results from this `EventLoopFuture`. | ||
| /// | ||
| /// This is useful when allowing users to provide promises for you to fulfill, but | ||
| /// when you are calling functions that return their own proimses. They allow you to | ||
| /// when you are calling functions that return their own promises. They allow you to | ||
| /// tidy up your computational pipelines. For example: | ||
| /// | ||
| /// ``` | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -40,7 +40,7 @@ final class LineDelimiterCodec: ByteToMessageDecoder { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// As we are using an `MultiThreadedEventLoopGroup` that uses more then 1 thread we need to ensure proper | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// synchronization on the shared state in the `ChatHandler` (as the same instance is shared across | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// child `Channel`s). For this a serial `DispatchQueue` is used when we modify the shared state (the `Dictonary`). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// child `Channel`s). For this a serial `DispatchQueue` is used when we modify the shared state (the `Dictionary`). | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// As `ChannelHandlerContext` is not thread-safe we need to ensure we only operate on the `Channel` itself while | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /// `Dispatch` executed the submitted block. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| final class ChatHandler: ChannelInboundHandler { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -119,7 +119,7 @@ let bootstrap = ServerBootstrap(group: group) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .serverChannelOption(ChannelOptions.backlog, value: 256) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Set the handlers that are appled to the accepted Channels | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Set the handlers that are applied to the accepted Channels | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .childChannelInitializer { channel in | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Add handler that will buffer data until a \n is received | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| channel.pipeline.add(handler: LineDelimiterCodec()).then { v in | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -177,7 +177,11 @@ let channel = try { () -> Channel in | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print("ChatServer started and listening on \(channel.localAddress!)") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if let localAddress = channel.localAddress { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print("ChatServer started and listening on \(localAddress)") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| print("ChatServer started but the address could not be determined.") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to change these: we will always know our local address at this stage.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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
This block contains three force-unwraps, but all of them are "safe" because of the Returning to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Lukasa agreed if you can know that it can never be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, if we really hate the force-unwrap, we can do
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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... I don't really know what to do here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 The new PR addressing this feedback is up here: #273 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // This will never unblock as we don't close the ServerChannel. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try channel.closeFuture.wait() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -498,7 +498,11 @@ let channel = try { () -> Channel in | |
| } | ||
| }() | ||
|
|
||
| print("Server started and listening on \(channel.localAddress!), htdocs path \(htdocs)") | ||
| 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)") | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to change these: we will always know our local address at this stage. |
||
|
|
||
| // This will never unblock as we don't close the ServerChannel | ||
| try channel.closeFuture.wait() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -224,9 +224,51 @@ defer { | |
| try! group.syncShutdownGracefully() | ||
| } | ||
|
|
||
| // First argument is the program path | ||
| let arguments = CommandLine.arguments | ||
| let arg1 = arguments.dropFirst().first | ||
| let arg2 = arguments.dropFirst(2).first | ||
|
|
||
| let channel = try! bootstrap.bind(host: "localhost", port: 8888).wait() | ||
| print("Server started and listening on \(channel.localAddress!)") | ||
| let defaultHost = "localhost" | ||
| let defaultPort = 8888 | ||
|
|
||
| enum BindTo { | ||
| case ip(host: String, port: Int) | ||
| case unixDomainSocket(path: String) | ||
| } | ||
|
|
||
| let bindTarget: BindTo | ||
| switch (arg1, arg1.flatMap(Int.init), arg2.flatMap(Int.init)) { | ||
| case (.some(let h), _ , .some(let p)): | ||
| /* we got two arguments, let's interpret that as host and port */ | ||
| bindTarget = .ip(host: h, port: p) | ||
|
|
||
| case (let portString?, .none, _): | ||
| // Couldn't parse as number, expecting unix domain socket path. | ||
| bindTarget = .unixDomainSocket(path: portString) | ||
|
|
||
| case (_, let p?, _): | ||
| // Only one argument --> port. | ||
| bindTarget = .ip(host: defaultHost, port: p) | ||
|
|
||
| default: | ||
| bindTarget = .ip(host: defaultHost, port: defaultPort) | ||
| } | ||
|
|
||
| let channel = try { () -> Channel in | ||
| switch bindTarget { | ||
| case .ip(let host, let port): | ||
| return try bootstrap.bind(host: host, port: port).wait() | ||
| case .unixDomainSocket(let path): | ||
| return try bootstrap.bind(unixDomainSocketPath: path).wait() | ||
| } | ||
| }() | ||
|
|
||
| if let localAddress = channel.localAddress { | ||
| print("Server started and listening on \(localAddress)") | ||
| } else { | ||
| print("Server started but the address could not be determined.") | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to change these: we will always know our local address at this stage. |
||
|
|
||
| // This will never unblock as we don't close the ServerChannel | ||
| try channel.closeFuture.wait() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mind removing this whitespace?