Thanks so much for doing this! This was meant as a todo item for us, or me, not you!
Cheers, Michael Sent from my iPhone > On 7 Aug 2020, at 18:25, Martin Duke <[email protected]> wrote: > > > OK, I'll file github issues > >> On Fri, Aug 7, 2020 at 1:21 AM Michael Welzl <[email protected]> wrote: >> Hi Martin, >> >> Thanks much for this detailed review! - and for the many interesting >> questions you’re raising; you discovered a few important holes there. E..g., >> I believe we just haven’t discussed stream limits; I would have thought that >> that’s an implementation matter, but the fact that you bring it up in a >> review of the interface draft is a reminder that applications may want to be >> aware of this behavior. >> (as a quick answer to this particular item, in my opinion Clone() should >> fail if the stream limit is exceeded - getting a mix of Connections and >> streams, altogether grouped is surely not what anyone would want or expect; >> e.g. priorities within the group won’t work out anymore). Note that the idea >> is that an application can query for what it got if it’s interested in that >> level of detail. >> >> Now, for addressing the comments, I think we should just split them up by >> topic into issues for the repository, and then address them one by one. >> >> Thanks again! >> >> Cheers, >> Michael >> >> >>> On Aug 6, 2020, at 9:30 PM, Martin Duke <[email protected]> wrote: >>> >>> Here is my promised "no prior knowledge" review of taps-interface. I >>> haven't filed issues for these yet, as I suspect some of these have already >>> been well-litigated there. >>> >>> The only other draft I've read is taps-arch. >>> >>> High level comments: >>> >>> (1) I think this conceptually headed in the right direction. >>> >>> (2) I found the relationship between streams and connections to be >>> extremely murky throughout the draft, and left it not really sure how I >>> would control a streamed transport. >>> >>> Section 6.4 hints suggests that opening additional streams is an >>> alternative implementation of Clone() for transports that can do so. Does >>> this imply that if the peer stream limit does not allow new streams, the >>> transport should open a connection instead? How does >>> SetNewConnectionLimit() in Sec 6.2 relate to streamed transport -- are >>> there two levels of listener: a stream listener and a connection listener? >>> How would opening "stream listeners" relate to the maximum number of >>> streams the peer could open (i.e. do I have to call listen() 8 times to >>> allow the client to open 8 streams)? Or are there no stream listeners and >>> these "connections" just sort of appear? >>> >>> Skimming the -impl draft, I can see there's some text here but it is not >>> fitting together for me. There are at least three streamed transports and >>> it would be helpful to explain how the API conceptualizes this, perhaps in >>> a new section. >>> >>> The next two major points are related to streams, so if I've missed >>> something important above they may be complete nonsequiturs. >>> >>> (3) I'm not sure the priority framework really fits together. >>> >>> Sec 6.4 says we 'should' do something that sounds like WFQ, but 7.1.5 >>> allows the application to decide what happens. Meanwhile message priority >>> suggests a strict priority-based scheduler. >>> >>> Meanwhile, in sec 8.1.3.2 "Note that this property is not a per-message >>> override of the connection Priority - see Section 7.1.3. Both Priority >>> properties >>> may interact, but can be used independently and be realized by >>> different mechanisms." >>> >>> At the risk of overindexing on a partly-baked draft in httpbis, how would I >>> implement strict priority of HTTP/3 streams over a taps API that implements >>> QUIC? Would I need to request strict priority in *both* connection priority >>> and message priority? Or I guess connection priority would be sufficient? >>> >>> (4) I found myself continually asking how I would implement foo-over-http/x >>> using this API, and very much stumbled over ambiguity regarding HTTP 1.1 >>> pipelining. While it's certainly possible to order messages in the proper >>> way using this API, if I was writing this agnostically to HTTP version, it >>> appears it would gravitate to opening a pooled TCP connection for each >>> request/response if we ended up on HTTP/1 [because each request/response >>> would have a clone() call]. Maybe this is OK, but I found this conclusion >>> to be jarring. >>> >>> Specific comments: >>> >>> sec 4.2.2 >>> "implementations MUST provide a >>> method for the application to determine the entire set of possible >>> values for each property." >>> >>> Is this meant to be a requirement for a machine-readable API for learning >>> the enum, or merely a requirement for documentation? i.e. is it satisfied >>> if the values are in the man page or comments of the C header file? >>> >>> sec 5. >>> I found it curious that there is no API here for registering callbacks, or >>> mention that this is a precondition to connection. Surely event handlers >>> are a prerequisite? >>> >>> sec 5.1 >>> I found it odd that there didn't appear to be any sort of formal list of >>> attributes of an endpoint, just some examples. >>> >>> sec 5.2.7 >>> I am not sure what this sentence means: "Disabling this property may enable >>> to control checksum coverage later." >>> >>> sec 5.2.9 >>> It would be helpful to say "Applications that neither prohibit nor require >>> congestion should query the result and disable their own congestion control >>> if present in the transport" >>> >>> sec 5.2.12 >>> Using temporary local addresses may break various mechanisms that prove >>> address ownership (e.g QUIC resumption tokens) and therefore impair >>> performance as the client has to re-verify its address. >>> >>> sec 6.2 >>> "After Stop() is called, the Listener can be disposed of.." I don't >>> understand the difference between calling listener.stop() and simply >>> disposing of the object. Does this imply that there is a listener.start() >>> that allows a configured listener to resume? >>> >>> sec 7.1.4 >>> It would be helpful to clarify the relevance of control vs application data >>> in the idle timeout -- for various reasons, the connection could stay alive >>> regardless of how active the connection is (in fact, perhaps "keepalive" is >>> a generally useful notion that can be exposed in the API?) >>> >>> sec 7.1..8 >>> This section is missing a reference to minSendRate and minRecvRate >>> >>> sec 7.1.9. >>> We're going to need an event for MTU changes. These can't simply be Send >>> errors because the Sent event might go first, or the transport might be >>> sending a probe with padding that doesn't correspond to a message. >>> >>> sec 7.2 >>> perhaps I'm not thinking of this correctly, but should there be a way to >>> query the peer-offered user timeout? >>> >>> sec 8.3.2.1 >>> Reading the Sent event, I was pretty sure that reliable transports would >>> deliver the event until the ack arrived. But Sec 10 most definitely does >>> not say that. I would think that the ack would be a more useful signal, but >>> maybe I'm in the rough on this -- certainly many transport implementations >>> will not do this well. >>> >>> -Martin >>> _______________________________________________ >>> Taps mailing list >>> [email protected] >>> https://www.ietf.org/mailman/listinfo/taps >>
_______________________________________________ Taps mailing list [email protected] https://www.ietf.org/mailman/listinfo/taps
