pjfanning opened a new pull request, #3510:
URL: https://github.com/apache/pekko/pull/3510
### Motivation
`deserializeCompressionAdvertisement` (`ArteryMessageSerializer:179-192`)
resolves every key in
the advertised table — for actor refs that means `resolveActorRef`, which
parses a path and
populates the per-thread resolve cache. The key list was unbounded, so the
only limit was the
transport frame size.
Measured on this branch:
```
256 entries -> 9009 wire bytes, 32 ms
1000 entries -> 35793 wire bytes, 37 ms
10000 entries -> 368793 wire bytes, 150 ms
50000 entries -> 1922409 wire bytes, 622 ms
```
256 is what a peer legitimately advertises (`compression.actor-refs.max`
defaults to 256), and
9 KB. The default `maximum-frame-size` of 256 KiB allows roughly 7000
entries, so the reachable
worst case is around 100 ms of CPU on the inbound control stream per
message, repeatable.
### Modification
Reject an advertisement carrying more entries than
`pekko.remote.artery.advanced.compression.<table>.max` — the setting that
bounds the table on
the sending side, parsed here the same way `ArterySettings` parses it. When
it is `"off"`
locally the setting is 0, there is no configured number to check against,
and no bound is
applied.
**The tradeoff worth a reviewer's attention:** this bounds by the
*receiver's* setting, but the
table is sized by the *sender's*. Configurations are normally uniform across
a cluster, but
during a rolling change of `actor-refs.max` a node still on the smaller
value would reject a
larger advertisement. I checked what that costs before choosing it: a
`fromBinary` failure on
the inbound stream is caught at `Codecs.scala:692-704`, which logs an error
and drops the
message — no quarantine — and `InboundCompressions` resends advertisements
periodically
(`InboundCompressions.scala:553`, "The ActorRefCompressionAdvertisement
message is resent
because it can be lost"). So the worst case for a skewed config is that
compression is not
established for that association until both sides are updated; messages
still flow uncompressed.
If reviewers would rather have headroom than exactness, the alternative is a
separate setting
with a default well above the table maxes.
### What I did not change
The review this came from also flagged `ActorRefResolveCache` as
hash-floodable:
`LruBoundedCache` never grows and `Unsafe.fastHash` is seeded with two fixed
public constants,
so colliding actor paths can be precomputed offline. I measured it rather
than assume:
generating 4096 keys that all land in one slot took 558 ms, and cache
operations then went from
2347 ns to 13631 ns, about 5.8x.
That is a real effect but far short of the degradation I expected, the cache
is bounded at 1024
entries so nothing grows without limit, and closing it means changing the
seed of a public
`Unsafe.fastHash`. 5.8x on one cache lookup does not seem to justify that,
so I left it alone
and am recording the numbers here instead.
### Result
An oversized advertisement is reported as a serialization failure. A
legitimately sized one is
unaffected.
### Tests
- `sbt "remote/testOnly org.apache.pekko.remote.serialization.*"` — 195
passed, 1 pending
One new test in `ArteryMessageSerializerSpec`, checked to discriminate by
reverting the
production file and re-running, where it fails with `no exception was
thrown`:
- `reject a compression table advertisement with more entries than the
configured maximum` —
asserts a table of exactly `max` entries still deserializes, and that `max
+ 1` is rejected
- `sbt "remote/mimaReportBinaryIssues"` — no issues
- `sbt "remote/scalafmtCheckAll" headerCreateAll` — clean
### References
Touches the same method as #3507, so whichever merges second needs a trivial
rebase; the two
guards are independent.
--
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]
