Responsiveness under Working Conditions #242
Conversation
Implementation of https://datatracker.ietf.org/doc/draft-ietf-ippm-responsiveness/ (draft 5) with flexible download and upload handlers to suit other use cases as well.
| import NIOCore | ||
| import NIOHTTPTypes | ||
|
|
||
| /// HTTP request handler sending a configurable stream of zeroes |
There was a problem hiding this comment.
Can we document that this requires the use of the HTTPTypes types, not the regular NIO ones.
There was a problem hiding this comment.
Can I suggest that, for now, we don't make the two "helper" handlers public? It's generally wise for us not to commit to too much API without a clear use-case. Making something public that was previously internal is generally easy, changing something that we accidentally made public is much harder.
There was a problem hiding this comment.
The two "helper" handlers you're referring to are the HTTPDrippingDownloadHandler and the HTTPReceiveDiscardHandler, right? The idea being only the ResponsivenessConfig and SimpleResponsivenessRequestMux end up being public?
I think doing that would end up making this more of an example than a building block. I find that request multiplexers are typically not very reusable (hence the prefix "Simple"). My expectation is that someone wanting to put together a robust implementation would have their own request mux and add things like limits on download size, delay durations, etc.
| // | ||
| //===----------------------------------------------------------------------===// | ||
|
|
||
| public struct ResponsivenessConfig: Encodable { |
There was a problem hiding this comment.
We should add the extra protocols to all of these. Codable, Hashable, Sendable.
| body: self.responsivenessConfigBuffer | ||
| ) | ||
| case (.get, .some(""), .some("responsiveness"), .some("download"), .some(let count)): | ||
| self.addHandlerOrInternalError(context: context, handler: HTTPDrippingDownloadHandler(count: 1, size: count)) |
There was a problem hiding this comment.
Are these args the wrong way around?
| self.addHandlerOrInternalError(context: context, handler: HTTPDrippingDownloadHandler(count: 1, size: count)) | |
| self.addHandlerOrInternalError(context: context, handler: HTTPDrippingDownloadHandler(count: count, size: 1)) |
There was a problem hiding this comment.
The ordering is correct. a single chunk of size N.The variable and parameter naming, however, is very confusing. Updated to try to be less confusing
|
|
||
| public func channelInactive(context: ChannelHandlerContext) { | ||
| self.phase = .done | ||
| self.scheduled?.cancel() |
There was a problem hiding this comment.
We should forward the channelInactive event as well
| if dataWritten { | ||
| context.flush() | ||
| } |
There was a problem hiding this comment.
Should we delay this slightly to avoid flushing twice in case we're done? (i.e. here and after writing end)
There was a problem hiding this comment.
Had to be careful here and put an extra if dataWritten inside one of the early returns, but the new code does avoid the unnecessary flush
| self.phase = .dripping(drippingState) | ||
| let this = NIOLoopBound(self, eventLoop: context.eventLoop) | ||
| let loopBoundContext = NIOLoopBound(context, eventLoop: context.eventLoop) | ||
| self.scheduled = context.eventLoop.scheduleTask(in: self.frequency) { |
There was a problem hiding this comment.
A recent-ish NIO got a scheduleCallback API which is slightly cheaper than scheduleTask: https://github.com/apple/swift-nio/blob/56f9b7c6fc9525ba36236dbb151344f8c35288df/Sources/NIOCore/EventLoop.swift#L378-L381
Might be worth using that.
There was a problem hiding this comment.
Thanks for the suggestion. I think I used it mostly correctly, but I'm not 100% sure. I haven't built much intuition around when swift does or doesn't allocate. I did find the fact that scheduleCallback throws a bit surprising and looked for an equivalent syncOperations-like method to avoid the try, but I didn't find one. I think try! may be fine in practice given current implementations, but not entirely correct. Interested in your feedback here.
| string: | ||
| "Received in excess of expectation; expected(\(expectation)) received(\(self.received))" | ||
| ) | ||
| self.writeSimpleResponse(context: context, status: .ok, body: body) |
There was a problem hiding this comment.
L46 says the status should be a 4xx status code.
Implementation of https://datatracker.ietf.org/doc/draft-ietf-ippm-responsiveness/ (draft 5) with flexible download and upload handlers to suit other use cases as well.
Motivation:
The provided handlers are useful for measuring responsiveness and testing things like performance of proxies
Modifications:
Add
NIOHTTPResponsivenessandNIOHTTPResponsivenessServerResult:
We'll now have an implementation of the Responsiveness under Working Conditions draft