Fixes bi-directional streaming #344

artemredkin · 2021-03-03T10:57:55Z

Motivation:
When we stream request body, current implementation expects that body
will finish streaming before we start to receive response body parts.
This is not correct, reponse body parts can start to arrive before we
finish sending the request.

Modifications:

Simplifies state machine, we only case about request being fully sent
to prevent sending body parts after .end, but response state machine
is mostly ignored and correct flow will be handled by NIOHTTP1
pipeline
Adds HTTPEchoHandler, that replies to each response body part
Adds bi-directional streaming test

Result:
Closes #327

artemredkin · 2021-03-03T11:05:35Z

Sources/AsyncHTTPClient/HTTPHandler.swift

@@ -1025,8 +1019,7 @@ extension TaskHandler: ChannelDuplexHandler {
                /// Some HTTP Servers can 'forget' to respond with CloseNotify when client is closing connection,
                /// this could lead to incomplete SSL shutdown. But since request is already processed, we can ignore this error.
                break
-            case .head where self.ignoreUncleanSSLShutdown,
-                 .body where self.ignoreUncleanSSLShutdown:
+            case .sending where self.ignoreUncleanSSLShutdown:


test testNoResponseWithIgnoreErrorForSSLUncleanShutdown fails if I add .sent, I'm not sure why yet...

.sending means that we have not sent client .end

What's the difference between .sent and .endOrError? In many cases (content-length present) after having sent the response body, we're done?

you are looking at slightly outdated code

this is a slightly weird part, it tries to do the following: if ignoreUncleanSSLShutdown is set and we received response .head, then it's allowed to not fail

weissi · 2021-03-03T11:20:44Z

Sources/AsyncHTTPClient/HTTPHandler.swift

@@ -840,23 +839,20 @@ extension TaskHandler: ChannelDuplexHandler {
            self.writeBody(request: request, context: context)
        }.flatMap {
            context.eventLoop.assertInEventLoop()
+
            if case .endOrError = self.state {


could we replace this if case and the fallthrough by one full switch self.state?

It's unclear to me why we can move from almost any state to .sent and why all those transitions are legal.

For example a transition from .sent to .sent seems odd, right? Or from .inactive to .sent seems wrong too.

weissi · 2021-03-03T11:21:22Z

Sources/AsyncHTTPClient/HTTPHandler.swift

-        case idle
-        case bodySent
+        case inactive
+        case sending


I think we could come up with slightly more prescriptive names? What are wen sending here? The request, the response, the head, the body, the trailers? Some of them, all of them?

Same for sent

weissi · 2021-03-03T11:23:52Z

Sources/AsyncHTTPClient/HTTPHandler.swift

@@ -902,7 +898,7 @@ extension TaskHandler: ChannelDuplexHandler {

    private func writeBodyPart(context: ChannelHandlerContext, part: IOData, promise: EventLoopPromise<Void>) {
        switch self.state {
-        case .idle:
+        case .sending:


this seems a little dangerous? Previously we could send in .idle and now we can't?

A full switch without the default would be better too here.

Sources/AsyncHTTPClient/HTTPHandler.swift

weissi · 2021-03-03T11:27:48Z

Sources/AsyncHTTPClient/HTTPHandler.swift

@@ -1025,8 +1019,7 @@ extension TaskHandler: ChannelDuplexHandler {
                /// Some HTTP Servers can 'forget' to respond with CloseNotify when client is closing connection,
                /// this could lead to incomplete SSL shutdown. But since request is already processed, we can ignore this error.
                break
-            case .head where self.ignoreUncleanSSLShutdown,
-                 .body where self.ignoreUncleanSSLShutdown:
+            case .sending where self.ignoreUncleanSSLShutdown:


What's the difference between .sent and .endOrError? In many cases (content-length present) after having sent the response body, we're done?

Tests/AsyncHTTPClientTests/HTTPClientTests.swift

weissi

thank you! This looks like a great start, left some comments

weissi · 2021-03-03T12:01:26Z

Sources/AsyncHTTPClient/HTTPHandler.swift

-        case sent
-        case head
+        case inactive
+        case sending_waiting


we should use lowerCamelCase to match swift's usual style.

I think we can do better than sendingWaiting, ie. what are we sending and what are we waiting for?

weissi · 2021-03-03T12:01:50Z

Sources/AsyncHTTPClient/HTTPHandler.swift

-        case bodySent
-        case sent
-        case head
+        case inactive


idle is probably better than inactive because the channel is actually isActive = true, right?

Lukasa

Seems reasonable enough to me, nothing obviously problematic in the code.

Sources/AsyncHTTPClient/HTTPHandler.swift

weissi · 2021-03-03T13:28:23Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+            case .sendingBodyResponseHeadReceived:
+                self.state = .bodySentResponseHeadReceived
+            case .bodySentWaitingResponseHead, .bodySentResponseHeadReceived:
+                preconditionFailure("should not happen")


should we print self.state in that message?

weissi · 2021-03-03T13:29:17Z

Sources/AsyncHTTPClient/HTTPHandler.swift

+                self.state = .bodySentResponseHeadReceived
+            case .bodySentWaitingResponseHead, .bodySentResponseHeadReceived:
+                preconditionFailure("should not happen")
+            case .endOrError, .redirected:


mind adding a comment why that's okay in .endOrError, or .redirected? I don't quite get how we can be in .end before we get the end of the body.

(similar for .redirected)

rewrote, added a test

Sources/AsyncHTTPClient/HTTPHandler.swift

Motivation: When we stream request body, current implementation expects that body will finish streaming _before_ we start to receive response body parts. This is not correct, reponse body parts can start to arrive before we finish sending the request. Modifications: - Simplifies state machine, we only case about request being fully sent to prevent sending body parts after .end, but response state machine is mostly ignored and correct flow will be handled by NIOHTTP1 pipeline - Adds HTTPEchoHandler, that replies to each response body part - Adds bi-directional streaming test Result: Closes swift-server#327

weissi

very nice, thank you!

artemredkin commented Mar 3, 2021

View reviewed changes

artemredkin force-pushed the fix_bi_streaming branch 3 times, most recently from ce06b45 to 88f4dd3 Compare March 3, 2021 11:45

weissi reviewed Mar 3, 2021

View reviewed changes

artemredkin force-pushed the fix_bi_streaming branch 2 times, most recently from 4b3c27d to 06a4098 Compare March 3, 2021 12:40

Lukasa approved these changes Mar 3, 2021

View reviewed changes

artemredkin force-pushed the fix_bi_streaming branch from 06a4098 to f49c143 Compare March 3, 2021 13:25

weissi reviewed Mar 3, 2021

View reviewed changes

artemredkin force-pushed the fix_bi_streaming branch from f49c143 to 26b1191 Compare March 3, 2021 16:47

weissi approved these changes Mar 3, 2021

View reviewed changes

weissi merged commit 5d9b784 into swift-server:main Mar 3, 2021

artemredkin deleted the fix_bi_streaming branch March 3, 2021 18:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes bi-directional streaming #344

Fixes bi-directional streaming #344

artemredkin commented Mar 3, 2021

artemredkin Mar 3, 2021

artemredkin Mar 3, 2021 •

edited

Loading

weissi Mar 3, 2021

artemredkin Mar 3, 2021

artemredkin Mar 3, 2021

weissi Mar 3, 2021

weissi Mar 3, 2021

artemredkin Mar 3, 2021

weissi Mar 3, 2021

artemredkin Mar 3, 2021

weissi Mar 3, 2021

weissi Mar 3, 2021

artemredkin Mar 3, 2021

weissi Mar 3, 2021

weissi left a comment

weissi Mar 3, 2021

artemredkin Mar 3, 2021

weissi Mar 3, 2021

artemredkin Mar 3, 2021

Lukasa left a comment

weissi Mar 3, 2021

artemredkin Mar 3, 2021

weissi Mar 3, 2021

weissi Mar 3, 2021

artemredkin Mar 3, 2021

weissi left a comment

Fixes bi-directional streaming #344

Fixes bi-directional streaming #344

Conversation

artemredkin commented Mar 3, 2021

Choose a reason for hiding this comment

artemredkin Mar 3, 2021 • edited Loading

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

weissi 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

Lukasa 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

weissi left a comment

Choose a reason for hiding this comment

artemredkin Mar 3, 2021 •

edited

Loading