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.
I want a way to "unread" a packet from an send queue, in the case where
I'm packing packets into a fixed space and don't know when I'm done
until I've read one too many packets. There's no way to insert the extra
packet at the head of the cannel representing the send queue, so that it
will be the next thing received from OutgoingQueue. Instead, add an
separate one-element queue called the stash. The caller can stash an
excess packet, then prioritize checking it in the next round by calling
Unstash before OutgoingQueue.
I don't know what I was thinking in
f1ee951fd6. The way it was written, if
there were not immediately additional packets to pack into the
downstream, it would stop trying to pack and would instead wait until
the maxResponseDelay or another response to send. What I meant is that
the timer and the next-response channel should have priority, if either
of those is true *and* there is additional downstream available to pack.
Only when both of those are false should we try to pack downstream data.
The server would log "NXDOMAIN: 0 bytes are too short to contain a
ClientID" even in the common cases where it got an A or NS query from
the resolver (possibly from QNAME minimization).
Not amount of raw payload. This allows for the case where the received
payload is only padding, for example. (That can't happen with the
current downstream encoding scheme, which doesn't allow for padding, so
I believe this change results in equivalent behavior.)
Previously I had maxEncodedPayload hard-coded as a separate constant,
but it is completely dependent on maxUDPPayload. We compute it by
actually constructing wire-format packets and taking their length, which
should be less fragile than the formula I previously had commented,
though it requires some synchronization between the sendLoop and
computeMaxEncodedPayload functions.
Formerly this was split into two pieces: one that did the domain check
and set the AA bit, and another that checked the AA bit and returned an
error if it was not set. It was done in two parts because the check for
UDP payload size occurred in the middle. Since 59f03791 the payload
check is moved to the end, so we can do the authoritative domain check
in one piece.
smux Stream.WriteTo may return io.EOF, which breaks the contract of
io.Copy that says it should not return io.EOF. smux.Stream doesn't have
a unidirectional shutdown, so we always end up slamming it shut in both
directions and leave the other direction with a broken pipe.
David Fifield