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.
Sources/NIOHTTPResponsiveness/SimpleResponsivenessRequestMux.swift
Outdated
Show resolved
Hide resolved
Sources/NIOHTTPResponsiveness/HTTPDrippingDownloadHandler.swift
Outdated
Show resolved
Hide resolved
| 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.
| // SAFTEY: scheduling the callback only potentially throws when invoked off eventloop | ||
| try! context.eventLoop.scheduleCallback(in: self.frequency, handler: self.scheduledCallbackHandler!) |
There was a problem hiding this comment.
Different EL implementations could throw for other reasons. In practice that probably won't be the case but it'd be safer to catch the error and call context.fireErrorCaught instead of try! here.
|
@Lukasa did you also want to review as you left a few comments before? |
|
Apologies for the delay here @ehaydenr, mind fixing up the conflicts for me? |
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