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
