Conversation
Use the standard if-zero-then-one sentinel pattern to avoid recomputing the hashCode on every call when the computed value happens to be 0. Co-Authored-By: Jules Ivanic <jules.ivanic@gmail.com>
✅ Deploy Preview for zio-http ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Updates SegmentCodec.hashCode caching to avoid the “computed hash is 0” sentinel collision, and adds tests around hashCode stability / non-zero behavior.
Changes:
- Remap computed hash
0to1when populating the cached hash code. - Add spec coverage asserting hashCode stability and that standard codecs produce non-zero hash codes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| zio-http/shared/src/main/scala/zio/http/codec/SegmentCodec.scala | Adjusts cached hash computation to remap 0 to 1 before caching. |
| zio-http/shared/src/test/scala/zio/http/codec/SegmentCodecSpec.scala | Adds tests asserting stable and non-zero hashCode behavior for segment codecs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test("hashCode is stable across multiple calls") { | ||
| val codec = SegmentCodec.literal("test") | ||
| val hash1 = codec.hashCode | ||
| val hash2 = codec.hashCode | ||
| assertTrue(hash1 == hash2, hash1 != 0) | ||
| }, | ||
| test("hashCode is non-zero for all standard segment codecs") { | ||
| val codecs: List[SegmentCodec[_]] = List( | ||
| SegmentCodec.empty, | ||
| SegmentCodec.literal("users"), | ||
| SegmentCodec.int("id"), | ||
| SegmentCodec.long("id"), | ||
| SegmentCodec.string("name"), | ||
| SegmentCodec.uuid("uuid"), | ||
| SegmentCodec.trailing, | ||
| ) | ||
| assertTrue(codecs.forall(_.hashCode != 0)) | ||
| }, |
There was a problem hiding this comment.
These tests don’t deterministically cover the specific regression this PR fixes (the computed hash being exactly 0 is astronomically unlikely, so the tests will almost always pass even without the remapping). To actually validate the behavior, consider making the raw hash computation injectable/overridable (e.g., a protected[codec] def computeHash: Int) so a test-only codec can force computeHash == 0 and assert the cached value becomes 1 and remains stable.
| if (_hashCode == 0) _hashCode = (this.getClass.getName(), render).hashCode | ||
| if (_hashCode == 0) { | ||
| val h = (this.getClass.getName(), render).hashCode | ||
| _hashCode = if (h == 0) 1 else h |
There was a problem hiding this comment.
The remap-to-1 behavior is a subtle sentinel workaround, but it’s not self-explanatory in code. Please add a short inline comment (or a named constant) explaining that 0 is reserved as the 'not computed' sentinel and is therefore remapped to avoid recomputation.
| _hashCode = if (h == 0) 1 else h | |
| _hashCode = if (h == 0) 1 else h // 0 is reserved as "not computed", so remap real hash 0 to 1 to avoid recomputation |
| @@ -45,7 +45,10 @@ sealed trait SegmentCodec[A] { self => | |||
| def format(value: A): Path | |||
|
|
|||
| override val hashCode: Int = { | |||
There was a problem hiding this comment.
The PR description discusses a lazily-initialized cached hashCode that can be recomputed on every call when the computed value is 0. In the diff, hashCode is overridden as a val, which is computed once at initialization time and won’t be recomputed on subsequent calls regardless of the sentinel value. Either adjust the implementation to match the described lazy-cached pattern (i.e., def hashCode with a cached var) or update the PR description/tests to match the eager val behavior.
Summary
if (h == 0) 1 else hcached hashCode sentinel pattern toSegmentCodec.hashCodehashCodecall when the computed value happens to be 0Background: the cached hashCode sentinel pattern
SegmentCodec.hashCodeuses the lazily-initialized cached hashCode pattern described in Joshua Bloch's Effective Java (Item 11, 3rd edition):The canonical real-world example is
java.lang.String.hashCode()in OpenJDK, which useshash == 0as the sentinel for "not yet computed."The one weakness of this pattern is that when the computed hash genuinely equals 0 (~1 in 2^32 probability), it will be recomputed on every call. The
if (h == 0) 1 else hrefinement remaps hash-0 to hash-1, eliminating even that edge case. The collision of mapping one extra value to 1 is negligible.Test plan