Hand-merge + squash of Piet De Vaere's giant PR. Fixes #37.

[ekr]

@pietdevaere: I hand-merged this and squashed it into one giant PR with a linear history. It passes tests, but could you please give it a once-over to see if it's OK.

[ekr]

@pietdevaere: would you mind making a PR against my the PR this branch is on? Then I'll merge that in and then merge both.

On Sat, Dec 2, 2017 at 11:11 AM, Piet De Vaere notifications@github.com wrote:

@pietdevaere approved this pull request.

Looks good. I didn't go in to much detail, but after this gets merged in to master, I will rebase my measurement code on top of it (to clean up the giant mess I made in my tree), and if any big issues slipped through they should pop up fast.

I made some tiny comments. I don't have write access, so you'll have to fix them.

In connection.go https://github.com/ekr/minq/pull/38#discussion_r154503794:

@@ -633,15 +662,18 @@ func (c Connection) sendQueued(bareAcks bool) (int, error) { func (c Connection) sendCombinedPacket(pt uint8, frames []frame, acks ackRanges, left int) (int, error) { asent := int(0) var err error

• for _, f := range frames {
• l, err := f.length()
• if err != nil {
• return 0, err
• }
• left -= l
• containsOnlyAcks := len(frames) == 0
• // See if there is space for any acks, and if there are acks waiting
• maxackblocks := (left - 16) / 5 // We are using 32-byte values for all the variable-lengths

This constant caught my eye. I think this is not right anymore. The ackFrame header is now 20 bytes, and all varaible-lenghts are 64-bit. (Unless I there is some magic happening in your encode functions)

In connection.go https://github.com/ekr/minq/pull/38#discussion_r154503801:

@@ -633,15 +662,18 @@ func (c Connection) sendQueued(bareAcks bool) (int, error) { func (c Connection) sendCombinedPacket(pt uint8, frames []frame, acks ackRanges, left int) (int, error) { asent := int(0) var err error

• for _, f := range frames {
• l, err := f.length()
• if err != nil {
• return 0, err
• }
• left -= l
• containsOnlyAcks := len(frames) == 0
• // See if there is space for any acks, and if there are acks waiting
• maxackblocks := (left - 16) / 5 // We are using 32-byte values for all the variable-lengths

Also, this should probably go in a constant

In connection.go https://github.com/ekr/minq/pull/38#discussion_r154503813:

@@ -633,15 +662,18 @@ func (c Connection) sendQueued(bareAcks bool) (int, error) { func (c Connection) sendCombinedPacket(pt uint8, frames []frame, acks ackRanges, left int) (int, error) { asent := int(0) var err error

• for _, f := range frames {
• l, err := f.length()
• if err != nil {
• return 0, err
• }
• left -= l
• containsOnlyAcks := len(frames) == 0
• // See if there is space for any acks, and if there are acks waiting
• maxackblocks := (left - 16) / 5 // We are using 32-byte values for all the variable-lengths
• if maxackblocks > 255 {
• maxackblocks = 255

This should probably go in a constant

In connection.go https://github.com/ekr/minq/pull/38#discussion_r154503816:

}

• if len(acks) > 0 {
• // (left - 16) is positive if there is place enough for a basic ACK frame without
• // aditional ACK blocks.
• if len(acks) > 0 && (left-16) >= 0 {

the 16 again

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ekr/minq/pull/38#pullrequestreview-80682041, or mute the thread https://github.com/notifications/unsubscribe-auth/ABD1oahsE9AQm_XZerEzd-hJo8CbWvdZks5s8aDwgaJpZM4QzVAo .

[pietdevaere]

ok