[PR] refactor: use bit operations for the DNS over TCP length prefix [pekko]

["pjfanning (via GitHub)" <gi...@apache.org>]

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: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org

Re: [PR] refactor: use bit operations for the DNS over TCP length prefix [pekko]

["pjfanning (via GitHub)" <gi...@apache.org>]

pjfanning merged PR #3466:
URL: https://github.com/apache/pekko/pull/3466


-- 
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: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org