STRLCPY/dnstt

Fix sending of leftover packets.

There was a logic error in the code. The nextP variable was used to
store the packet that was too big to pack into the most recent DNS
response. But nextP was not tied to any particular ClientID; instead it
would be sent to whatever client happened to be the recipient of the
next response.

The confusion didn't cause connections to fail completely; any
misdirected packets were treated as out-of-sequence garbage by KCP and
dropped. But it hurt performance a lot: I saw a download go from 300
KB/s to 50 KB/s just by connecting a second client with a different
ClientID (not even sending or receiving with the second client). The
reason is that a fraction of the packets intended for the downloading
client were instead sent to the idle client, which to the downloading
client looks like a packet drop, requiring a retransmission by the
server.

We fix it by placing the leftover packet in a per-ClientID stash, rather
than a variable shared by all ClientIDs.

David Fifield committed 4 years ago

8f965fe3

1 parent 938463ce

Revision indexing in progress... (symbol navigation in revisions will be accurate after indexed)

Total 1 files

■ ■ ■ ■ ■ ■

dnstt-server/main.go

		skipped 540 lines
541	541		// fit while keeping the total size under maxEncodedPayload, then sends it.
542	542		func sendLoop(dnsConn net.PacketConn, ttConn turbotunnel.QueuePacketConn, ch <-chan record, maxEncodedPayload int) error {
543	543		var nextRec *record
544		-	var nextP []byte
545	544		for {
546	545		rec := nextRec
547	546		nextRec = nil
		skipped 23 lines
571	570		}
572	571
573	572		var payload bytes.Buffer
574		-
575	573		limit := maxEncodedPayload
576		-	if len(nextP) > 0 {
577		-	// No length check on any packet left over from
578		-	// the previous bundle -- if it's too large, we
579		-	// allow it to be truncated and dropped.
580		-	limit -= 2 + len(nextP)
581		-	binary.Write(&payload, binary.BigEndian, uint16(len(nextP)))
582		-	payload.Write(nextP)
583		-	}
584		-	nextP = nil
585		-
586		-	// We loop and write as many packets from OutgoingQueue
	574	+	// We loop and bundle as many packets from OutgoingQueue
587	575		// into the response as will fit. Any packet that would
588		-	// overflow the capacity of the DNS response, we save in
589		-	// nextP to be included in a future response.
	576	+	// overflow the capacity of the DNS response, we stash
	577	+	// to be bundled into a future response.
590	578		timer := time.NewTimer(maxResponseDelay)
591	579		loop:
592	580		for {
	581	+	var p []byte
593	582		select {
594		-	// Prioritize the first two cases over the
595		-	// OutgoingQueue case. The first two cases are
596		-	// duplicated under the default case.
	583	+	// Check the nextRec, timer, and stash cases
	584	+	// before considering the OutgoingQueue case.
	585	+	// Only if all these cases fail do we enter the
	586	+	// default arm, where they are checked again in
	587	+	// addition to OutgoingQueue.
597	588		case nextRec = <-ch:
598		-	// If there's another response
599		-	// waiting to be sent, wait no
600		-	// longer for a payload for this
601		-	// one.
	589	+	// If there's another response waiting
	590	+	// to be sent, wait no longer for a
	591	+	// payload for this one.
602	592		break loop
603	593		case <-timer.C:
604	594		break loop
	595	+	case p = <-ttConn.Unstash(rec.ClientID):
605	596		default:
606	597		select {
607	598		case nextRec = <-ch:
608	599		break loop
609	600		case <-timer.C:
610	601		break loop
611		-	case p := <-ttConn.OutgoingQueue(rec.ClientID):
612		-	// We wait for the first packet
613		-	// in a bundle only. The second
614		-	// and later packets must be
615		-	// immediately available or they
616		-	// will be omitted from this
617		-	// send.
618		-	timer.Reset(0)
	602	+	case p = <-ttConn.Unstash(rec.ClientID):
	603	+	case p = <-ttConn.OutgoingQueue(rec.ClientID):
	604	+	}
	605	+	}
	606	+	// We wait for the first packet in a bundle
	607	+	// only. The second and later packets must be
	608	+	// immediately available or they will be omitted
	609	+	// from this bundle.
	610	+	timer.Reset(0)
619	611
620		-	if int(uint16(len(p))) != len(p) {
621		-	panic(len(p))
622		-	}
623		-	if 2+len(p) > limit {
624		-	// Save this packet to
625		-	// send in the next
626		-	// response.
627		-	nextP = p
628		-	break loop
629		-	}
630		-	limit -= 2 + len(p)
631		-	binary.Write(&payload, binary.BigEndian, uint16(len(p)))
632		-	payload.Write(p)
633		-	}
	612	+	limit -= 2 + len(p)
	613	+	if payload.Len() == 0 {
	614	+	// No packet length check for the first
	615	+	// packet; if it's too large, we allow
	616	+	// it to be truncated and dropped by the
	617	+	// receiver.
	618	+	} else if limit < 0 {
	619	+	// Stash this packet to send in the next
	620	+	// response.
	621	+	ttConn.Stash(p, rec.ClientID)
	622	+	break loop
634	623		}
	624	+	if int(uint16(len(p))) != len(p) {
	625	+	panic(len(p))
	626	+	}
	627	+	binary.Write(&payload, binary.BigEndian, uint16(len(p)))
	628	+	payload.Write(p)
635	629		}
636	630		timer.Stop()
637	631
		skipped 268 lines