Call `didSendRequestPart` at the right time #566

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.

Sign up for GitHub

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

Jump to bottom

Merged

fabianfett merged 4 commits into main from fb-didSendRequestPart-promise

Apr 26, 2022

Collaborator

FranzBusch commented Mar 16, 2022

Motivation

Right now, we call didSendRequestPart after passing the write to the executor. However, this does not mean that the write hit the socket. To implement proper backpressure using the delegate, we should only call this method once the write was successful.

Modification

Pass a promise to the actual channel write and only call the delegate once that promise succeeds.

Result

The delegate method didSendRequestPart is only called after the write was successful.

swift-server-bot commented Mar 16, 2022

Can one of the admins verify this patch?

7 similar comments

swift-server-bot commented Mar 16, 2022

Can one of the admins verify this patch?

swift-server-bot commented Mar 16, 2022

Can one of the admins verify this patch?

swift-server-bot commented Mar 16, 2022

Can one of the admins verify this patch?

swift-server-bot commented Mar 16, 2022

Can one of the admins verify this patch?

swift-server-bot commented Mar 16, 2022

Can one of the admins verify this patch?

swift-server-bot commented Mar 16, 2022

Can one of the admins verify this patch?

swift-server-bot commented Mar 16, 2022

Can one of the admins verify this patch?

FranzBusch force-pushed the fb-didSendRequestPart-promise branch from ef29eb0 to ebafa1e Compare

March 16, 2022 10:57

Collaborator

dnadoba commented Mar 16, 2022

@swift-nio-bot add to allowlist

FranzBusch force-pushed the fb-didSendRequestPart-promise branch 2 times, most recently from a60554e to 5780c10 Compare

March 17, 2022 09:09

FranzBusch marked this pull request as ready for review

March 17, 2022 09:09

FranzBusch requested review from fabianfett, glbrntt and Lukasa

March 17, 2022 09:09

Lukasa requested changes

View reviewed changes

Collaborator

Lukasa left a comment

There needs to be much clearer auditing about who is responsible for succeeding this promise. A bunch of these calls return without either storing the promise or passing it on, and in those cases we risk leaking the promise without succeeding it. That all needs to be updated.

Collaborator Author

FranzBusch commented Mar 18, 2022

@Lukasa Thanks for your review. You absolutely write I just dropped the promise in a couple of places. I audited the code paths again and made sure to fail the promise in the other branches and added some tests.

The only thing that I wasn't really sure about is what errors to return in some places. Looking forward to suggestions!

FranzBusch requested a review from Lukasa

March 18, 2022 09:37

Lukasa reviewed

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/HTTPClient.swift Outdated

                   public static let readTimeout = HTTPClientError(code: .readTimeout)
                   /// Remote connection was closed unexpectedly.
                   public static let remoteConnectionClosed = HTTPClientError(code: .remoteConnectionClosed)
+                  /// Request failed.
+                  public static let failed = HTTPClientError(code: .failed)

Collaborator

Lukasa Mar 24, 2022

Can we call this requestFailed instead of just failed?

Collaborator Author

FranzBusch Mar 24, 2022

I was aligning it with the naming of cancelled which is documented as a cancelled request. What do you prefer?

Member

fabianfett Apr 14, 2022

Please document, that we use this failure, to fail write promises on failed requests. This shall not be used as a general purpose error, even though the name is general purpose.

Sources/AsyncHTTPClient/RequestBag.swift

-                          self.delegate.didSendRequestPart(task: self.task, part)
+                          let promise = self.task.eventLoop.makePromise(of: Void.self)
+                          promise.futureResult.whenSuccess {
+                              self.delegate.didSendRequestPart(task: self.task, part)

Collaborator

Lukasa Mar 24, 2022

Should we report an error to the delegate in the failure case?

Collaborator Author

FranzBusch Mar 24, 2022

Was thinking about that as well. Open to input here. @weissi For your backpressure implementation would this be valuable?

Member

fabianfett Mar 24, 2022

We should make sure the error delegate is only called once.

Member

fabianfett Apr 20, 2022

Should we report an error to the delegate in the failure case?

I don't think we should. All errors are reported to the error function.

FranzBusch force-pushed the fb-didSendRequestPart-promise branch from 47c3087 to 373792c Compare

April 13, 2022 12:45

FranzBusch requested a review from Lukasa

April 13, 2022 12:47

FranzBusch force-pushed the fb-didSendRequestPart-promise branch from 373792c to b549617 Compare

April 13, 2022 13:29

Lukasa approved these changes

View reviewed changes

Collaborator

Lukasa left a comment

LGTM but I'd like to see @fabianfett or @dnadoba review.

Collaborator Author

FranzBusch commented Apr 13, 2022

The API breakage is failing because of the two new protocol requirements. However, I added default implementations in an extension so this is not a breaking change.

fabianfett reviewed

View reviewed changes

Member

fabianfett left a comment

Thanks so much for driving this. Left a bunch of comments.

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift Outdated Show resolved Hide resolved

Tests/AsyncHTTPClientTests/HTTP2ClientRequestHandlerTests.swift Outdated

Comment on lines 373 to 377

+                      requestHandler.writeRequestBodyPart(
+                          .byteBuffer(.init()),
+                          request: requestBag,
+                          promise: promise
+                      )

Member

fabianfett Apr 14, 2022

This tests doesn't make any sense to me. The requestHandler is not on an EmbeddedChannel. I think what we actually want to test here, is the delegate called, once and only once the write has hit the beginning of the ChannelPipeline. For this you will need a handler that holds the write for a bit, before it is allowed to let the writes through.

Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift Outdated Show resolved Hide resolved

Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift Outdated

+                      XCTAssertEqual(state.requestCancelled(), .failRequest(HTTPClientError.cancelled, .none))
+                      XCTAssertEqual(state.requestStreamPartReceived(part, promise: promise), .wait,
+                                     "Expected to drop all stream data after having received a response head, with status >= 300")
+                      XCTAssertThrowsError(try promise.futureResult.wait())

Member

fabianfett Apr 14, 2022

Please verify the correct error here.

Tests/AsyncHTTPClientTests/Mocks/MockRequestExecutor.swift Outdated

Comment on lines 196 to 199

+                      if self.shouldFailWrites {
+                          promise?.fail(ChannelError.eof)
+                      } else {
+                          promise?.succeed(())
+                      }

Member

fabianfett Apr 14, 2022

You just passed the promise to another internal write method, but then fail it or succeed it here? That looks wrong.

Tests/AsyncHTTPClientTests/Mocks/MockRequestExecutor.swift Show resolved Hide resolved

FranzBusch force-pushed the fb-didSendRequestPart-promise branch 2 times, most recently from 82d51cb to 944acc6 Compare

April 20, 2022 07:39

FranzBusch requested a review from fabianfett

April 20, 2022 07:39

fabianfett reviewed

View reviewed changes

Member

fabianfett left a comment

Looks much better now. However we should change one more critical piece.

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift Outdated

    
                      case .wait:

                          break

                      case .wait(let promise):

                          promise?.fail(ChannelError.eof)

Member

fabianfett Apr 20, 2022

Urgh. I feel like this is a little to wild...

I don't think we should add a hidden an action into the wait/none action. I think we should add a special action for this case, instead of extending wait.
Why ChannelError.eof? I think we should create a new internal error for this. HTTPRequestUploadCancelledBecauseResponseGT300. Also the error should be created in the state machine and handed out from the state machine in the new action with the promise.

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Outdated

    
                      case .wait:

                          break

                      case .wait(let promise):

                          promise?.fail(ChannelError.eof)

Member

fabianfett Apr 20, 2022

See HTTP2 handler.

Sources/AsyncHTTPClient/RequestBag.swift

-                          self.delegate.didSendRequestPart(task: self.task, part)
+                          let promise = self.task.eventLoop.makePromise(of: Void.self)
+                          promise.futureResult.whenSuccess {
+                              self.delegate.didSendRequestPart(task: self.task, part)

Member

fabianfett Apr 20, 2022

Should we report an error to the delegate in the failure case?

I don't think we should. All errors are reported to the error function.

FranzBusch requested a review from fabianfett

April 20, 2022 14:19

fabianfett requested changes

View reviewed changes

Member

fabianfett left a comment

Found some more things. But this is starting to look, really good!

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTPRequestStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/RequestBag.swift Outdated

Comment on lines 174 to 180

+                      case .forwardStreamFinished(let writer, let writerPromise):
+                          let promise = self.task.eventLoop.makePromise(of: Void.self)
+                          promise.futureResult.whenSuccess {
+                              self.delegate.didSendRequest(task: self.task)
+                          }
+                          writer.finishRequestBodyStream(self, promise: promise)
+                          writerPromise?.succeed(())

Member

fabianfett Apr 21, 2022

Why do we create a new promise? Why is the writePromise just succeeded? This looks odd. This needs at least a comment.

Collaborator Author

FranzBusch Apr 22, 2022

I thought that we need a new promise here but I just tested it again and with using the same promise and it worked.

FranzBusch force-pushed the fb-didSendRequestPart-promise branch 2 times, most recently from a1c2ebf to 926e482 Compare

April 22, 2022 07:08

FranzBusch requested a review from fabianfett

April 22, 2022 07:08

FranzBusch force-pushed the fb-didSendRequestPart-promise branch from 926e482 to 76b051a Compare

April 22, 2022 07:12

FranzBusch commented

View reviewed changes

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Outdated

+                                  oldRequest.succeedRequest(buffer)
+                              }
+                              context.close(promise: writePromise)

Collaborator Author

FranzBusch Apr 22, 2022

@fabianfett Is it okay to pass the writePromise to close here?

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Outdated

-                              context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
-                          case .informConnectionIsIdle:
+                          case .close(let writePromise):
+                              context.close(promise: writePromise)

Collaborator Author

FranzBusch Apr 22, 2022

Same question here @fabianfett ?

Member

fabianfett Apr 22, 2022

I don't think it makes sense to pass the writePromise to close. Closing the connection really just concerns the standing connection and not the running request. failing the promise below is correct IMO.

FranzBusch force-pushed the fb-didSendRequestPart-promise branch 2 times, most recently from 8cbc71c to 2d28426 Compare

April 22, 2022 08:35

fabianfett reviewed

View reviewed changes

Member

fabianfett left a comment

And another round!

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Outdated

-                              context.writeAndFlush(self.wrapOutboundOut(.end(nil)), promise: nil)
-                          case .informConnectionIsIdle:
+                          case .close(let writePromise):
+                              context.close(promise: writePromise)

Member

fabianfett Apr 22, 2022

I don't think it makes sense to pass the writePromise to close. Closing the connection really just concerns the standing connection and not the running request. failing the promise below is correct IMO.

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift

    
                      case failRequest(Error, FinalStreamAction)

                      case succeedRequest(FinalStreamAction, CircularBuffer<ByteBuffer>)

                      case failRequest(Error, FinalFailedStreamAction)

                      case succeedRequest(FinalSuccessfulStreamAction, CircularBuffer<ByteBuffer>)

Member

fabianfett Apr 22, 2022

Not an issue introduced by you. Urgh. I think we should change the order here (first buffer, than last action). Maybe as a follow up.

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP2/HTTP2ClientRequestHandler.swift Outdated Show resolved Hide resolved

Tests/AsyncHTTPClientTests/HTTPRequestStateMachineTests.swift Outdated Show resolved Hide resolved


          Call didSendRequestPart at the right time

85bb24d

# Motivation
Right now, we call `didSendRequestPart` after passing the write to the executor. However, this does not mean that the write hit the socket. To implement proper backpressure using the delegate, we should only call this method once the write was successful.

# Modification
Pass a promise to the actual channel write and only call the delegate once that promise succeeds.

# Result
The delegate method `didSendRequestPart` is only called after the write was successful.

FranzBusch force-pushed the fb-didSendRequestPart-promise branch from 2d28426 to 85bb24d Compare

April 25, 2022 12:22

FranzBusch requested a review from fabianfett

April 25, 2022 12:22

fabianfett reviewed

View reviewed changes

Member

fabianfett left a comment

Nearly done!

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ClientChannelHandler.swift Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift Outdated Show resolved Hide resolved

Sources/AsyncHTTPClient/ConnectionPool/HTTP1/HTTP1ConnectionStateMachine.swift Outdated Show resolved Hide resolved


          Code review

80ff383

FranzBusch requested a review from fabianfett

April 25, 2022 15:49

fabianfett approved these changes

View reviewed changes

Member

fabianfett left a comment

Thank you! This has been a ton of work!

fabianfett added the 🔨 semver/patch label

FranzBusch and others added 2 commits

April 26, 2022 10:31


          Fix test

d7f5ff9


          Merge branch 'main' into fb-didSendRequestPart-promise

1bf1526

fabianfett merged commit 2442598 into main

fabianfett deleted the fb-didSendRequestPart-promise branch

April 26, 2022 14:04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch