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

Add RowStreamStateMachine #176

Merged
merged 1 commit into from
Sep 18, 2021

Conversation

fabianfett
Copy link
Collaborator

Motivation

To best support back-pressure, we extract the necessary state machine into a dedicated substate machine.

Changes

  • Add RowStreamStateMachine

Note

This pr does not include any tests and the new code will not be hit. We will add tests to the ExtendedQueryStateMachine that test the new functionality once we wire the new substate machine up.

@fabianfett fabianfett added enhancement New feature or request part of a series labels Sep 18, 2021
@fabianfett fabianfett requested a review from gwynne September 18, 2021 12:27
Copy link
Member

@gwynne gwynne left a comment

Choose a reason for hiding this comment

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

LGTM , one style nit I didn't even handle correctly anyway 😅

Comment on lines 37 to 62
case .waitingForRows(var buffer):
self.state = .modifying
buffer.append(newRow)
self.state = .waitingForRows(buffer)

// For all the following cases, please note:
// Normally these code paths should never be hit. However there is one way to trigger
// this:
//
// If the server decides to close a connection, NIO will forward all outstanding
// `channelRead`s without waiting for a next `context.read` call. For this reason we might
// receive new rows, when we don't expect them here.
case .waitingForRead(var buffer):
self.state = .modifying
buffer.append(newRow)
self.state = .waitingForRows(buffer)

case .waitingForDemand(var buffer):
self.state = .modifying
buffer.append(newRow)
self.state = .waitingForRows(buffer)

case .waitingForReadOrDemand(var buffer):
self.state = .modifying
buffer.append(newRow)
self.state = .waitingForRows(buffer)
Copy link
Member

Choose a reason for hiding this comment

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

Feel free to "tweak" my "code style" (generous to call it that, I agree) as you find apropos, I mostly just care about the code de-duplication.

Suggested change
case .waitingForRows(var buffer):
self.state = .modifying
buffer.append(newRow)
self.state = .waitingForRows(buffer)
// For all the following cases, please note:
// Normally these code paths should never be hit. However there is one way to trigger
// this:
//
// If the server decides to close a connection, NIO will forward all outstanding
// `channelRead`s without waiting for a next `context.read` call. For this reason we might
// receive new rows, when we don't expect them here.
case .waitingForRead(var buffer):
self.state = .modifying
buffer.append(newRow)
self.state = .waitingForRows(buffer)
case .waitingForDemand(var buffer):
self.state = .modifying
buffer.append(newRow)
self.state = .waitingForRows(buffer)
case .waitingForReadOrDemand(var buffer):
self.state = .modifying
buffer.append(newRow)
self.state = .waitingForRows(buffer)
case .waitingForRows(var buffer),
// For all the following cases, please note:
// Normally these code paths should never be hit. However there is one way to trigger
// this:
//
// If the server decides to close a connection, NIO will forward all outstanding
// `channelRead`s without waiting for a next `context.read` call. For this reason we might
// receive new rows, when we don't expect them here.
.waitingForRead(var buffer),
.waitingForDemand(var buffer),
.waitingForReadOrDemand(var buffer):
self.state = .modifying
buffer.append(newRow)
self.state = .waitingForRows(buffer

}

mutating func end() -> CircularBuffer<PSQLBackendMessage.DataRow> {
switch self.state {
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, like this! This is better than my suggestion above, do it this way there too!

@fabianfett fabianfett force-pushed the ff-add-rowstreamstatemachine branch from 0260b86 to 748ee70 Compare September 18, 2021 13:16
@fabianfett fabianfett merged commit ce57b02 into vapor:main Sep 18, 2021
@fabianfett fabianfett deleted the ff-add-rowstreamstatemachine branch September 18, 2021 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants