pjfanning opened a new pull request, #3466:
URL: https://github.com/apache/pekko/pull/3466
### Motivation
RFC 1035 section 4.2.2 prefixes each DNS over TCP message with its length as
two big endian bytes. Both directions in `TcpDnsClient` expressed this with
arithmetic rather than bit operations:
```scala
def encodeLength(length: Int): ByteString =
ByteString((length / 256).toByte, length.toByte)
def decodeLength(data: ByteString): Int =
((data(0).toInt + 256) % 256) * 256 + ((data(1) + 256) % 256)
```
The `+ 256) % 256` pairs are working around `Byte` being signed, which `&
0xFF` states directly, and the division and multiplication are a shift by 8.
### Modification
```scala
def encodeLength(length: Int): ByteString =
ByteString(((length >> 8) & 0xFF).toByte, (length & 0xFF).toByte)
def decodeLength(data: ByteString): Int =
((data(0) & 0xFF) << 8) | (data(1) & 0xFF)
```
Plus a short comment naming the RFC section, since the two byte big endian
framing is not obvious from the code alone.
### This is behaviour preserving, not a fix
I want to be clear that this fixes nothing — the original was correct. I
verified equivalence exhaustively rather than by inspection:
- `decodeLength`: identical over **all 65,536 byte pairs** (every `(Byte,
Byte)` combination), producing the full `[0, 65535]` range in both forms.
- `encodeLength`: identical over the **entire valid length range 0 to
65535**.
The two forms diverge in exactly one place: `encodeLength(-1)` gives `(0,
-1)` under the old form and `(-1, -1)` under the new one. That cannot occur —
the only caller is `encodeLength(bytes.length)` at `TcpDnsClient.scala:62`,
where `bytes` is a `ByteString`, so the argument is never negative. I checked
the call site rather than assuming.
So the case for merging is readability, and a small amount of work removed
from the receive path (two divisions and a multiplication become a shift and
two masks). If you would rather not take churn in working DNS framing code for
that, this is a reasonable one to decline.
### Tests
Added a `"The TCP DNS length prefix"` block to `TcpDnsClientSpec` covering
the helpers directly, which nothing previously did — they were only exercised
indirectly through the client's receive loop:
- round trip over **every** representable length, 0 to 65535
- the expected big endian byte layout at the boundaries (0, 1, 255, 256,
65535)
- decoding of bytes with the high bit set (`0xFF 0xFF` → 65535, `0x80 0x80`
→ 32896) — the case the signed-byte workaround existed for
- trailing bytes after the two byte prefix are ignored
**These pass against both the old and the new implementation.** That is the
correct outcome for a behaviour preserving change, and worth stating plainly:
unlike a bug fix, these tests do not fail on the unmodified code. Their value
is locking the contract down so a future change to this framing cannot silently
break it.
`actor-tests/testOnly org.apache.pekko.io.dns.internal.TcpDnsClientSpec` — 8
tests, all pass. `sbt actor/mimaReportBinaryIssues` clean; `TcpDnsClient` is
`private[internal]`, so no API surface is affected. `scalafmt` run on both
modules.
### References
Found during a sweep for patterns left over from the Java 8 / Scala 2.12
baseline, alongside #3465.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
