`NIOSSLHandler`: behave sensibly on `close(mode: .output)` #428

felixschlegel · 2023-05-15T10:20:02Z

Motivation

If NIOSSLHandler receives close(mode: .output) or close(mode: .input), it ignores them, optionally failing their promises. This is weird behaviour. It should do better.

Modifications

flush outstanding messages on NIOSSLHandler.close(mode: .output)
close SSL Connection on NIOSSLHandler.close(mode: .output)
support closing the input of NIOSSLHandler when used as a server with ChannelOptions.Types.AllowRemoteHalfClosureOption enabled
trigger TCP close when NIOSSLHandler.state == .inputClose
write integration tests to verify the behaviour

Warning: This PR relies on EmbeddedChannel supporting ChannelOptions.Types.AllowRemoteHalfClosureOption, which is currently not possible and proposed as a Patch to swift-nio

Note: I have removed a lot of whitespace, so consider selecting Hide Whitespace in the GitHub Files Changed tab

Sources/NIOSSL/NIOSSLHandler.swift

Lukasa · 2023-05-19T12:13:24Z

Sources/NIOSSL/NIOSSLHandler.swift

    }

    public func channelActive(context: ChannelHandlerContext) {
+        context.channel.getOption(ChannelOptions.allowRemoteHalfClosure).whenSuccess { halfClosureAllowed in


I don't think caching this value is semantically quite right: users can change this value during the lifetime of the Channel. We should look it up when we need it.

Similarly, we should try to use the sync options where we can.

Lukasa · 2023-05-19T12:14:01Z

Sources/NIOSSL/NIOSSLHandler.swift

    }
-
+
+    // TODO: how do I test this?


With EmbeddedChannel, you can fire the input closed event into the pipeline manually.

Lukasa · 2023-05-19T12:14:56Z

Sources/NIOSSL/NIOSSLHandler.swift

+            userInboundInputClosedTriggered(context: context)
+        default:
+            context.fireUserInboundEventTriggered(event)
+            // Trigger closure of TCP input as this event has not occured although we are in state .inputClosed already.


This code seems to be in the wrong place.

Lukasa · 2023-05-19T12:25:56Z

Sources/NIOSSL/NIOSSLHandler.swift

+        let channelError: NIOSSLError
+        switch self.state {
+        case .closed, .idle:
+            return


In both of these cases we should probably still send the user event onwards.

Sources/NIOSSL/NIOSSLHandler.swift

Lukasa · 2023-05-19T12:37:59Z

Sources/NIOSSL/NIOSSLHandler.swift

+            // We flush all outstanding writes once the handshake step is complete and set our state to .outputClosed aftwerwards.
+            // This prevents any further writes to this channel.
+            if let promise = promise, let closeOutputPromise = self.closeOutputPromise {
+                closeOutputPromise.futureResult.cascade(to: promise)


Can we possibly have a closeOutputPromise if we're not in state closingOutput?

Lukasa · 2023-05-19T12:39:10Z

Sources/NIOSSL/NIOSSLHandler.swift

+            } else if let promise = promise {
+                self.closeOutputPromise = promise
+                self.state = .closingOutput(self.scheduleTimedOutShutdown(context: context))
+                self.doShutdownStep(context: context)


It seems wrong to only do the shutdown if there's a promise.

Sources/NIOSSL/NIOSSLHandler.swift

Lukasa · 2023-05-19T12:40:45Z

Sources/NIOSSL/NIOSSLHandler.swift

+
+            self.flush(context: context)
+
+            self.closeOutputPromise?.futureResult.whenSuccess {


This is incorrect: if we don't have a promise, this never fires.

This makes me thing that maybe outputClosing isn't necessary at all. It's a sort of immediate transition: we send our CLOSE_NOTIFY and the process is conceptually "done". So maybe this is an atomic transition.

Ok, I have added two new commits, one removing the .outputClosing state and one removing the closeOutputPromise — let me know what you think!

Sources/NIOSSL/NIOSSLHandler.swift

Lukasa · 2023-05-26T15:23:19Z

Sources/NIOSSL/NIOSSLHandler.swift

-                    self.state = .closing(self.scheduleTimedOutShutdown(context: context))
-                case .unwrapping, .closing:
+                case .active, .outputClosed:
+                    if self.getAllowRemoteHalfClosureFromChannel(context: context) == false {


Ideally we'd cache this across the call to doFlushReadData.

Good point!

Lukasa · 2023-05-26T15:24:01Z

Sources/NIOSSL/NIOSSLHandler.swift

+        var halfClosureAllowed = false
+        if let syncOptions = context.channel.syncOptions {
+            let result = try? syncOptions.getOption(ChannelOptions.allowRemoteHalfClosure)
+            result.map { halfClosureAllowed = $0 }


Using map for the side-effect is a bit weird here, it's probably better to use if let.

Lukasa · 2023-05-26T15:25:36Z

Sources/NIOSSL/NIOSSLHandler.swift


+    /// Completes the given promise when all outstanding writes have been flushed.
+    private func bufferedWritesCascadeFlushPromise(
+        to promise: EventLoopPromise<Void>?,


I don't think this use of the promise is correct. It should be attached to the close(mode: .output) operation. We should fail it if we don't actually issue that call, but we shouldn't succeed it ourselves.

Sources/NIOSSL/NIOSSLHandler.swift

Lukasa · 2023-05-31T12:53:36Z

Sources/NIOSSL/NIOSSLHandler.swift

+            flushCompletePromise.futureResult.whenComplete { _ in
+                self.doShutdownStep(context: context)
+            }
+            bufferedWritesCascadeFlushPromise(to: flushCompletePromise, context: context)


So this promise dance is probably unnecessary. We can send closeOutput once we've called flush, even if the write promises haven't completed. Handlers are required to queue close(mode: .output) behind writes.

Lukasa · 2023-05-31T12:55:34Z

Sources/NIOSSL/NIOSSLHandler.swift

                completeHandshake(context: context)
            }
-        case .unwrapping, .closing, .unwrapped, .closed:
+        case .unwrapping, .closing, .unwrapped, .closed, .inputClosed, .outputClosed:


I think this is wrong, .inputClosed and .outputClosed should match active here I think.

felixschlegel · 2023-06-01T09:56:59Z

Hey @Lukasa ,

I have implemented your requested changes and added the buffering of closeOutput when we are still in state = .handshaking / .additionalVerification.

I still have a couple of open questions for this PR:

should we also support scheduledShutdown for close(mode: .output) (see the normal close mode for reference)
what if the downstream ChannelHandler does not support close(mode: .output)? At the moment we just invoke context.close(mode: .output) hoping for the next ChannelHandler not to fail

Best regards,
-Felix

Lukasa · 2023-06-08T13:49:59Z

@swift-server-bot add to allowlist

Lukasa · 2023-06-08T13:51:30Z

should we also support scheduledShutdown for close(mode: .output) (see the normal close mode for reference)

No, scheduled shutdown is waiting for us to get a CLOSE_NOTIFY from the remote peer. In this mode, we don't expect one.

what if the downstream ChannelHandler does not support close(mode: .output)?

Then the partial close fails and the user should upgrade to full close.

Lukasa · 2023-06-08T13:56:20Z

Sources/NIOSSL/NIOSSLHandler.swift

-            let bufferedWrite = self.bufferedWrites.removeFirst()
-            bufferedWrite.promise?.fail(reason)
+    private func discardBufferedActions(reason: Error) {
+        while self.bufferedActions.count > 0 {


This can be more concisely expressed as while let bufferedAction = self.bufferedActions.popFirst().

Lukasa · 2023-06-08T13:57:31Z

Sources/NIOSSL/NIOSSLHandler.swift

+                    }
+                    return writeSuccessful
+                case .closeOutput:
+                    invokeCloseOutput = true


This looks like it incorrect reorders a close(mode: .output) with writes that follow it. Once we get close(mode: .output) we should stop iterating the buffered actions and then, after we send the close, discard anything left in the buffer.

Lukasa · 2023-06-08T13:57:49Z

Sources/NIOSSL/NIOSSLHandler.swift

        } catch {
            // We encountered an error, it's cleanup time. Close ourselves down.
-            channelClose(context: context, reason: error)
+            if case .outputClosed = self.state {} else {


This is a really weird construction, prefer using a switch.

Modifications: * flush outstanding messages on NIOSSLHandler.close(mode: .output) * change state once NIOSSLHandler.close(mode: .output) is invoked, so that subsequent writes to the channel fail * write integration tests to verify the behaviour

…utput)

…ctive(.output)" This reverts commit 59cf0df.

…LHandler

Modifications: * send user event "input closed" onwards when in state .closed or .idle * trigger closure of TCP input once we enter the .inputClosed state

Modifications: * in `closeOuput` trigger the shutdown step regardless of the existence of a promise * remove redundant state transition to `.outputClosed`

Modifications: * remove `.closingOutput` state

Modifications: * pass on user inbound event "input closed" to the next `ChannelHandler` event in the event of an error * cache value of `ChannelOptions.allowRemoteHalfClosure` across `doFlushReadData` to avoid races in case the option's value changes while `doFlushReadData` is being executed * change behaviour: pass `closeOutputPromise` on in `context.close(mode: .output, promise: closeOutputPromise`

Modifications: * `NIOSSLHandler.userInboundInputClosedTriggered`: execute `fireErrorCaught` before `userInboundEventTriggered(.inputClosed)` * `func completedAdditionalPeerCertificateVerification` fails with invalid state when in states `{.input,.ouput}closed`

Modifications: * create `enum BufferedAction` that allows `NIOSSLHandler` to buffer actions instead of just writes * rename `bufferedWrites` -> `bufferdActions` * add `BufferedAction.closeOutput`, which allows us to buffer invocations of `close(mode: .output)` when we are still in non-active modes such as `.handshaking` or `.additionalVerification` -> this means we'll invoke the closing of the output after our handshaking step is completed and all our buffered writes have been flushed

Modifications: * Refactoring * fixed wrong comment * remove `MarkedCircularBuffer.forEachElementUntilMark()` * discard all buffered actions that occured after the `.closeOutput` buffered actions in `NIOSSLHandler.doUnbufferActions()`

Motivation: Previously we did not check the current state when setting the `NIOSSLHandler`'s state to `.inputClosed`. This resulted in an issue where when we were in state `.closing`, then set the state to `.inputClosed`, we would end up with a fatalError in our shutdown / scheduledShutdown as state `.inputClosed` is not a shutting-down state. Modifications: * check current state before setting `state = .inputClosed`, specifically not setting the state to `.inputClosed` when we are already in the process of doing a full closure

felixschlegel · 2023-06-23T12:20:27Z

@swift-server-bot test this please

yim-lee · 2023-06-23T15:53:59Z

@swift-server-bot test this please

Modifications: * `NIOSSLHandler`: escalate to full closure when both input and output are closed * `NIOSSLIntegrationTest.testCloseModeOutputServerAndClient`: assert that having input and output closed in `NIOSSLHandler` results in full closure * `NIOSSLIntegrationTest.testCloseModeOutputServerAndClient`: assert that if one side closes its output, the other side of the connection closes its input

Lukasa · 2023-06-26T12:34:28Z

Sources/NIOSSL/NIOSSLHandler.swift

            switch targetCompleteState {
+            case .outputClosed:
+                // No full channel close here. We would expect users to invoke a full close regardless of
+                // previously completed half closures.


This comment is out of date now, right?

Thanks for pointing that out! I have updated the comment so that it reflects the current state of our half-closure behaviour 😄

Lukasa · 2023-06-26T12:37:04Z

Sources/NIOSSL/NIOSSLHandler.swift

-            channelClose(context: context, reason: error)
+            switch self.state {
+            case .outputClosed:
+                break


Why do we suppress error handling in this state?

Modifications: * update comment on why we don't do a full channel close when we execute `doShutDownStep` for state `.outputClosed` * do not suppress error in `NIOSSLHandler.doUnbufferActions` when in state `.outputClosed`

felixschlegel force-pushed the fs-new-close-mode-behaviour branch 3 times, most recently from 267422f to 59cf0df Compare May 15, 2023 15:45

felixschlegel commented May 15, 2023

View reviewed changes

Sources/NIOSSL/NIOSSLHandler.swift Show resolved Hide resolved

felixschlegel commented May 15, 2023

View reviewed changes

Sources/NIOSSL/NIOSSLHandler.swift Outdated Show resolved Hide resolved

felixschlegel marked this pull request as ready for review May 15, 2023 16:09

felixschlegel marked this pull request as draft May 16, 2023 09:28

felixschlegel force-pushed the fs-new-close-mode-behaviour branch 2 times, most recently from 4616900 to 416465c Compare May 17, 2023 10:17

felixschlegel mentioned this pull request May 17, 2023

Embedded: getOption(ChannelOptions.allowRemoteHalfClosure) should not fatalError apple/swift-nio#2429

Merged

felixschlegel marked this pull request as ready for review May 18, 2023 13:55

Lukasa added 🔨 semver/patch No public API change. 🆕 semver/minor Adds new public API. and removed 🔨 semver/patch No public API change. labels May 19, 2023

Lukasa reviewed May 19, 2023

View reviewed changes

felixschlegel force-pushed the fs-new-close-mode-behaviour branch 2 times, most recently from 5ed780a to 41ebe30 Compare May 23, 2023 10:26

felixschlegel requested a review from Lukasa May 24, 2023 09:48

Lukasa reviewed May 26, 2023

View reviewed changes

felixschlegel requested a review from Lukasa May 30, 2023 13:30

Lukasa reviewed May 31, 2023

View reviewed changes

felixschlegel requested a review from Lukasa June 1, 2023 09:42

Lukasa reviewed Jun 8, 2023

View reviewed changes

felixschlegel and others added 4 commits June 23, 2023 13:13

* replace ConnectionState.closedOutput with ConnectionState.active(.o…

d05c56d

…utput)

Revert "* replace ConnectionState.closedOutput with ConnectionState.a…

9bf17c4

…ctive(.output)" This reverts commit 59cf0df.

close(mode: .output): perform TLS shutdown

571c3c3

felixschlegel added 19 commits June 23, 2023 13:13

Add NIOSSLIntegrationTest.testMultipleCLoseOutput()

3939f3b

Trigger TCP close when NIOSSLHandler.state == .inputClose

8432b71

testCloseModeOutputTriggersFlush: add allowHalfClosure to serverChannel

7a682ee

* don't cache value of ChannelOptions.allowRemoteHalfClosure in NIOSS…

ef1ad2e

…LHandler

Refactor

966e1eb

Refactor TCP input close handling

2e73a18

Modifications: * send user event "input closed" onwards when in state .closed or .idle * trigger closure of TCP input once we enter the .inputClosed state

Refactor closingOutput procedure

b16bfb0

Modifications: * in `closeOuput` trigger the shutdown step regardless of the existence of a promise * remove redundant state transition to `.outputClosed`

* closeOutput: flush before we doShutdown

5cd37d1

Refactor closeOutput state management

e5a8b80

Modifications: * remove `.closingOutput` state

* closeOutput: remove closeOutputPromise

e4e4447

* remove TCP half close example

a297c8f

* add comment about not closing the entire channel after a half-closure

fcba7f1

* rework .inputClosed and .outputClosed state

b2362f1

* add test for closing server output and client output

06ce747

Review Cory

90717be

Modifications: * `NIOSSLHandler.userInboundInputClosedTriggered`: execute `fireErrorCaught` before `userInboundEventTriggered(.inputClosed)` * `func completedAdditionalPeerCertificateVerification` fails with invalid state when in states `{.input,.ouput}closed`

Review Cory

5ce00f1

Modifications: * Refactoring * fixed wrong comment * remove `MarkedCircularBuffer.forEachElementUntilMark()` * discard all buffered actions that occured after the `.closeOutput` buffered actions in `NIOSSLHandler.doUnbufferActions()`

felixschlegel force-pushed the fs-new-close-mode-behaviour branch from 494fb31 to 93458ef Compare June 23, 2023 12:14

felixschlegel requested a review from Lukasa June 23, 2023 12:22

felixschlegel force-pushed the fs-new-close-mode-behaviour branch from 93458ef to 937b25b Compare June 26, 2023 09:52

Lukasa reviewed Jun 26, 2023

View reviewed changes

Review Cory

62b8ef3

Modifications: * update comment on why we don't do a full channel close when we execute `doShutDownStep` for state `.outputClosed` * do not suppress error in `NIOSSLHandler.doUnbufferActions` when in state `.outputClosed`

felixschlegel requested a review from Lukasa June 26, 2023 13:06

Lukasa approved these changes Jun 26, 2023

View reviewed changes

Lukasa merged commit 678b641 into apple:main Jun 26, 2023


		self.flush(context: context)

		self.closeOutputPromise?.futureResult.whenSuccess {

NIOSSLHandler: behave sensibly on close(mode: .output) #428

NIOSSLHandler: behave sensibly on close(mode: .output) #428

Uh oh!

Conversation

felixschlegel commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felixschlegel commented Jun 1, 2023

Uh oh!

Lukasa commented Jun 8, 2023

Uh oh!

Lukasa commented Jun 8, 2023

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!

felixschlegel commented Jun 23, 2023

Uh oh!

yim-lee commented Jun 23, 2023

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

`NIOSSLHandler`: behave sensibly on `close(mode: .output)` #428

`NIOSSLHandler`: behave sensibly on `close(mode: .output)` #428

felixschlegel commented May 15, 2023 •

edited

Loading