Skip to content

Add a RequestQueue #412

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

Merged
merged 4 commits into from
Sep 1, 2021
Merged

Conversation

fabianfett
Copy link
Member

Motivation

For our Connection Pool we need to queue requests.

Changes

  • Remove previously added Waiter type... Not needed anymore.
  • Add Request box
  • Add RequestQueue

Result

Less code in the actual state machine.

@fabianfett fabianfett added this to the HTTP/2 support milestone Aug 31, 2021
@fabianfett fabianfett added 🔨 semver/patch No public API change. semver/none No version bump required. and removed 🔨 semver/patch No public API change. labels Aug 31, 2021
@fabianfett fabianfett force-pushed the ff-pool-state-queue branch from ddc0233 to 77809cc Compare August 31, 2021 21:03
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

I think the queue has some surprising behaviour that should be changed or at least made much clearer.

self.generalPurposeQueue.isEmpty
}

mutating func count(for eventLoop: EventLoop?) -> Int {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think as a user I'd be very surprised that getting the count is mutating and potentially allocates!

Perhaps there should be a non-mutating version of withEventLoopQueue, something like:

func withEventLoopQueueIfAvailable<Result>(
    for eventLoopID: EventLoopID, 
    _ closure: (CircularBuffer<Request>) -> Result
) -> Result?

return self.generalPurposeQueue.count
}

mutating func isEmpty(for eventLoop: EventLoop?) -> Bool {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here re: mutating


mutating func removeAll() -> [Request] {
var result = [Request]()
result = self.eventLoopQueues.reduce(into: result) { partialResult, element in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this just eventLoopQueues.flatMap { $1 }?

Comment on lines 96 to 98
self.generalPurposeQueue.forEach { request in
result.append(request)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use result.append(contentsOf: self.generalPurposeQueue) to avoid intermediate reallocs of result

import NIOEmbedded

/// An `EventLoopGroup` of `EmbeddedEventLoop`s.
final class EmbeddedEventLoopGroup: EventLoopGroup {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What a great idea!

Comment on lines 28 to 34
var count: Int {
self.generalPurposeQueue.count
}

var isEmpty: Bool {
self.generalPurposeQueue.isEmpty
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is surprising behaviour I think -- I would expect the count to return the total count and isEmpty to only return true if both queues are empty.

}
}

func __testOnly_internal_value() -> HTTPSchedulableRequest {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: "wrapped request" or similar would be clearer than internal_value

@fabianfett fabianfett requested a review from glbrntt September 1, 2021 09:19
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks good modulo a couple of nits

self.count = 0
}

private(set) var count: Int
Copy link
Collaborator

Choose a reason for hiding this comment

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

How often is this used externally? If it's not used frequently then dynamically computing this would be better I think...

Comment on lines 125 to 126
if self.eventLoopQueues[eventLoopID] != nil {
return closure(self.eventLoopQueues[eventLoopID]!)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: avoid hashing twice here (also we can avoid the bang)

@fabianfett fabianfett requested a review from glbrntt September 1, 2021 11:05
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

LGTM

@fabianfett fabianfett merged commit 1f5b633 into swift-server:main Sep 1, 2021
@fabianfett fabianfett deleted the ff-pool-state-queue branch September 1, 2021 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants