Skip to content
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

Enable command pipelining #552

Merged
merged 11 commits into from
Apr 29, 2021
Merged

Conversation

Davidde94
Copy link
Collaborator

Resolves #528

Previously the user wasn't able to write a command while another was waiting for continuations. This isn't very friendly, and isn't great In any real-world scenario. Now we queue commands, and as soon as all continuations for one command have been handled, we move on to the next.

@Davidde94 Davidde94 requested a review from weissi April 23, 2021 12:16
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this won't work for certain cases. We should probably fix this or at the very least detect them and crash (and file an issue).

Sources/NIOIMAP/Coders/IMAPClientHandler.swift Outdated Show resolved Hide resolved
Sources/NIOIMAP/Coders/IMAPClientHandler.swift Outdated Show resolved Hide resolved
@Davidde94 Davidde94 requested a review from weissi April 28, 2021 17:08
Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments, there are certainly a few new re-entrancy bugs here. And I don't think we have a write re-entrancy test case.

Sources/NIOIMAP/Coders/IMAPClientHandler.swift Outdated Show resolved Hide resolved
context.write(self.wrapOutboundOut(next.bytes), promise: nil)
} else {
self.currentEncodeBuffer = nil
context.write(self.wrapOutboundOut(next.bytes), promise: promise)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't there a lot of logic duplicated here from above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh this was already here but I can factor it out

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

@Davidde94
Copy link
Collaborator Author

@weissi I've filed an issue so we don't forget about the re-entrancy issues : #555

@Davidde94 Davidde94 requested a review from weissi April 29, 2021 12:25
self.currentEncodeBuffer = (commandEncoder._buffer, promise)
assert(self.state == .expectingResponses)
self.state = .expectingLiteralContinuationRequest
context.write(self.wrapOutboundOut(next.bytes), promise: nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't right. I think if this context.write fails here, we'll never fail the user promise (and will therefore crash). If the user gave us a promise, you'll need to .cascadeFailure into the user's promise.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

& test please

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code didn't change here again, but I can add the cascade

context.write(self.wrapOutboundOut(next.bytes), promise: nil)
} else {
self.currentEncodeBuffer = nil
context.write(self.wrapOutboundOut(next.bytes), promise: promise)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please

Sources/NIOIMAP/Coders/IMAPClientHandler.swift Outdated Show resolved Hide resolved
@@ -348,6 +414,55 @@ class IMAPClientHandlerTests: XCTestCase {
self.writeOutbound(.command(.init(tag: "A1", command: .idleStart)))
}

// func testProtectAgainstReentrancyWithContinuation() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that the one you want to fix in a separate issue? Happy with that as an exception

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I can't really claim I fully understand this code anymore. The interaction between self._bufferedCommands, self.currentEncodeBuffer, and the state machine bits that are inline with the channel handler code is very complex. Also we already know of one bug (I think) which is hit by the commented test.

But I think we can get this in to unblock folks assuming the deficiencies are properly filed in issues. I'd recommend refactoring this in two bits:

  1. state machine which does all the state handling and returns an "action"
  2. the channel handler implementation which just drives the state machine and calls out to the others via the channel pipeline.

This will then also allow testing the state machine without setting up channels, channel handlers etc.

@Davidde94 Davidde94 merged commit 2f538c6 into apple:main Apr 29, 2021
@Davidde94 Davidde94 deleted the de/command-pipelining branch April 29, 2021 16:09
@danieleggert
Copy link
Collaborator

So I can't really claim I fully understand this code anymore. The interaction between self._bufferedCommands, self.currentEncodeBuffer, and the state machine bits that are inline with the channel handler code is very complex. Also we already know of one bug (I think) which is hit by the commented test.

But I think we can get this in to unblock folks assuming the deficiencies are properly filed in issues. I'd recommend refactoring this in two bits:

  1. state machine which does all the state handling and returns an "action"
  2. the channel handler implementation which just drives the state machine and calls out to the others via the channel pipeline.

This will then also allow testing the state machine without setting up channels, channel handlers etc.

@Davidde94 Are you tracking this refactoring in another issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users can't queue more than one command at a time
3 participants