allow two distinct ChannelOptions of one type #597

weissi · 2018-08-28T12:07:48Z

Motivation:

Quite embarrasingly, we previously would only store one ChannelOption
per ChannelOption type. Most channel option types are distinct and
that's probably why it took so long to find this issue. Thanks
@pushkarnk for reporting. Unfortunately though, the most important
ChannelOption is .socket which crucially also holds a level and a
name. That means if you set two ChannelOptions.socket options with
distinct name/level, one would still override the other.

Example:

.serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEPORT), value: 1)
.serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1)

would only actually set the latter.

Modifications:

made all common ChannelOption types equatable (for 2.0 this will
be a protocol requirement)
deprecated non-Equatable ChannelOption types

Result:

you can now set two distinct ChannelOptions for one type

normanmaurer

generally LGTM... a few things

normanmaurer · 2018-08-28T14:34:19Z

Sources/NIO/Bootstrap.swift

    public func serverChannelOption<T: ChannelOption>(_ option: T, value: T.OptionType) -> Self {
        serverChannelOptions.put(key: option, value: value)
+        print(serverChannelOptions)


normanmaurer · 2018-08-28T14:34:27Z

Sources/NIO/Bootstrap.swift

+    ///     - value: The value for the option.
+    public func serverChannelOption<T: ChannelOption & Equatable>(_ option: T, value: T.OptionType) -> Self {
+        serverChannelOptions.put(key: option, value: value)
+        print(serverChannelOptions)


normanmaurer · 2018-08-28T14:35:20Z

Sources/NIO/Bootstrap.swift

@@ -672,11 +720,34 @@ public final class DatagramBootstrap {
    }
 }

-fileprivate struct ChannelOptionStorage {
+internal struct ChannelOptionStorage {


Add comment that its internal for testing only

normanmaurer · 2018-08-28T14:36:32Z

Tests/NIOTests/DatagramChannelTests.swift

+        }
+        XCTAssertNoThrow(XCTAssertGreaterThan(try channel.getOption(option: ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET),
+                                                                                                  SO_REUSEADDR)).wait(),
+        0))


formatting looks really odd... maybe merge with previous line ?

normanmaurer · 2018-08-28T14:36:37Z

Tests/NIOTests/DatagramChannelTests.swift

+        0))
+        XCTAssertNoThrow(XCTAssertGreaterThan(try channel.getOption(option: ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET),
+                                                                                                  SO_DEBUG)).wait(),
+                                              0))


formatting looks really odd... maybe merge with previous line ?

normanmaurer · 2018-08-28T14:36:53Z

Tests/NIOTests/ChannelTests.swift

+                                              0))
+        XCTAssertNoThrow(XCTAssertGreaterThan(try server3.getOption(option: ChannelOptions.socket(IPPROTO_TCP,
+                                                                                                  TCP_NODELAY)).wait(),
+                                              0))


formatting looks really odd... maybe merge with previous line ? (same comment for others)

@normanmaurer then the line would be too long. And in github you couldn't see any of the important bits as they were hidden off screen. Still want me to merge?

maybe use this formatting then ?:

let value = XCTAssertNoThrow(try server3.getOption(option: ChannelOptions.socket(IPPROTO_TCP,TCP_NODELAY)).wait()) XCTAssertGreaterThan(value, 0))

just put the shared code in a function actually

normanmaurer · 2018-08-28T15:06:54Z

Tests/NIOTests/ChannelTests.swift

+        }
+        XCTAssertNoThrow(XCTAssertGreaterThan(try server.getOption(option: ChannelOptions.socket(SOL_SOCKET,
+                                                                                                 SO_REUSEADDR)).wait(),
+                                              0))


can you also do the same refactoring here ?

normanmaurer · 2018-08-28T16:52:51Z

Sources/NIO/Bootstrap.swift

+    }
+
+    // HACK: this function should go for NIO 2.0, all ChannelOptions should be equatable
+    mutating func put<K: ChannelOption>(key: K, value: K.OptionType) {


Please remember to open an issue once we merge this one

normanmaurer · 2018-08-28T17:49:53Z

Sources/NIO/BaseSocket.swift

@@ -332,7 +332,10 @@ class BaseSocket: Selectable {
    ///     - value: The value for the option.
    /// - throws: An `IOError` if the operation failed.
    final func setOption<T>(level: Int32, name: Int32, value: T) throws {
-        try withUnsafeFileDescriptor { fd in
+        if level == SocketOptionValue(IPPROTO_TCP) && name == TCP_NODELAY && (try? self.localAddress().protocolFamily) == Optional<Int32>.some(Int32(Posix.AF_UNIX)) {


Can you explain why we need to ignore these ? Maybe adding a comment would be good.

@normanmaurer done that, also #599

@pushkarnk

Motivation: Quite embarrasingly, we previously would only store one `ChannelOption` per `ChannelOption` type. Most channel option types are distinct and that's probably why it took so long to find this issue. Thanks @pushkarnk for reporting. Unfortunately though, the most important `ChannelOption` is `.socket` which crucially also holds a level and a name. That means if you set two `ChannelOptions.socket` options with distinct name/level, one would still override the other. Example: .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEPORT), value: 1) .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1) would only actually set the latter. Modifications: - made all common `ChannelOption` types equatable (for 2.0 this will be a protocol requirement) - deprecated non-Equatable `ChannelOption` types - zero out buffer before calling getsockopt as Linux doesn't do that Result: you can now set two distinct `ChannelOptions` for one type

normanmaurer

LGTM

@pushkarnk

Motivation: Quite embarrasingly, we previously would only store one `ChannelOption` per `ChannelOption` type. Most channel option types are distinct and that's probably why it took so long to find this issue. Thanks @pushkarnk for reporting. Unfortunately though, the most important `ChannelOption` is `.socket` which crucially also holds a level and a name. That means if you set two `ChannelOptions.socket` options with distinct name/level, one would still override the other. Example: .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEPORT), value: 1) .serverChannelOption(ChannelOptions.socket(SocketOptionLevel(SOL_SOCKET), SO_REUSEADDR), value: 1) would only actually set the latter. Modifications: - made all common `ChannelOption` types equatable (for 2.0 this will be a protocol requirement) - deprecated non-Equatable `ChannelOption` types - zero out buffer before calling getsockopt as Linux doesn't do that Result: you can now set two distinct `ChannelOptions` for one type Motivation: Explain here the context, and why you're making that change. What is the problem you're trying to solve. Modifications: Describe the modifications you've done. Result: After your change, what will change.

Motivation: While we fixed a bug in SwiftNIO about setting multiple options of the same type in apple/swift-nio#597, we never brought a fix for that issue forward. We did aim to resolve this in SwiftNIO 2.0 by providing ChannelOptions.Storage, but unfortunately we forgot to make the initializer for that type public (see apple/swift-nio#988). While SwiftNIO core has to get its ducks in a row, users of this library should still be able to set more than one socket option, in my humble opinion. Modifications: - Ported over ChannelOptions.Storage until apple/swift-nio#988 is fixed. - Transitioned our code to use it. - Tested it works. Result: Users can set multiple channel options.

Motivation: We never brought across the fix to apple/swift-nio#597 into swift-nio-transport-services. This has caused some users a few hard-to-track-down surprises. We fixed this in the mainline by way of apple#40, but users running the old codebase probably want this fix too. Modifications: - Side-ported the fix from apple/swift-nio#597. - Back-ported the test from apple#40. Result: Users can set multiple socket options.

Motivation: While we fixed a bug in SwiftNIO about setting multiple options of the same type in apple/swift-nio#597, we never brought a fix for that issue forward. We did aim to resolve this in SwiftNIO 2.0 by providing ChannelOptions.Storage, but unfortunately we forgot to make the initializer for that type public (see apple/swift-nio#988). While SwiftNIO core has to get its ducks in a row, users of this library should still be able to set more than one socket option, in my humble opinion. Modifications: - Ported over ChannelOptions.Storage until apple/swift-nio#988 is fixed. - Transitioned our code to use it. - Tested it works. Result: Users can set multiple channel options.

Motivation: We never brought across the fix to apple/swift-nio#597 into swift-nio-transport-services. This has caused some users a few hard-to-track-down surprises. We fixed this in the mainline by way of #40, but users running the old codebase probably want this fix too. Modifications: - Side-ported the fix from apple/swift-nio#597. - Back-ported the test from #40. Result: Users can set multiple socket options.

weissi requested a review from normanmaurer August 28, 2018 12:07

weissi mentioned this pull request Aug 28, 2018

Bind errors while running Kitura and Kitura-NIO tests Kitura/Kitura-NIO#2

Closed

weissi force-pushed the jw-two-opts-one-type branch from dc8e9d8 to aa10523 Compare August 28, 2018 14:32

normanmaurer requested changes Aug 28, 2018

View reviewed changes

weissi force-pushed the jw-two-opts-one-type branch from aa10523 to bc3f576 Compare August 28, 2018 15:03

normanmaurer reviewed Aug 28, 2018

View reviewed changes

weissi force-pushed the jw-two-opts-one-type branch 3 times, most recently from e1d77cf to 51532c1 Compare August 28, 2018 16:39

weissi added this to the 1.9.3 milestone Aug 28, 2018

weissi added the semver/patch No public API change. label Aug 28, 2018

weissi force-pushed the jw-two-opts-one-type branch from 51532c1 to 4765aae Compare August 28, 2018 16:46

normanmaurer reviewed Aug 28, 2018

View reviewed changes

normanmaurer approved these changes Aug 28, 2018

View reviewed changes

weissi force-pushed the jw-two-opts-one-type branch 2 times, most recently from 0244764 to 2938130 Compare August 28, 2018 17:47

normanmaurer requested changes Aug 28, 2018

View reviewed changes

weissi force-pushed the jw-two-opts-one-type branch from 2938130 to 19966aa Compare August 28, 2018 18:12

weissi mentioned this pull request Aug 29, 2018

RegressionTests.testBadRequestFollowingGoodRequest fails with #78 Kitura/Kitura-NIO#81

Closed

normanmaurer approved these changes Aug 29, 2018

View reviewed changes

weissi merged commit 501af86 into apple:master Aug 29, 2018

weissi deleted the jw-two-opts-one-type branch August 29, 2018 14:26

nethraravindran mentioned this pull request Aug 29, 2018

Set SO_REUSEADDR for all socket channels to avoid bind errors Kitura/Kitura-NIO#82

Merged

Lukasa mentioned this pull request May 1, 2019

Allow setting multiple options of the same type. apple/swift-nio-transport-services#40

Merged

Lukasa mentioned this pull request May 1, 2019

Support setting multiple socket options. apple/swift-nio-transport-services#41

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow two distinct ChannelOptions of one type #597

allow two distinct ChannelOptions of one type #597

weissi commented Aug 28, 2018

normanmaurer left a comment

normanmaurer Aug 28, 2018

normanmaurer Aug 28, 2018

normanmaurer Aug 28, 2018

normanmaurer Aug 28, 2018

normanmaurer Aug 28, 2018

normanmaurer Aug 28, 2018

weissi Aug 28, 2018

normanmaurer Aug 28, 2018

weissi Aug 28, 2018

normanmaurer Aug 28, 2018

normanmaurer Aug 28, 2018

weissi Aug 28, 2018

normanmaurer Aug 28, 2018

weissi Aug 28, 2018

normanmaurer left a comment

allow two distinct ChannelOptions of one type #597

allow two distinct ChannelOptions of one type #597

Conversation

weissi commented Aug 28, 2018

normanmaurer 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

normanmaurer left a comment

Choose a reason for hiding this comment