[serve] Read the multiplex marker statically so probing cannot initialize handles - #65064
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the use of getattr with inspect.getattr_static when checking for the multiplexed function marker in _callable_uses_multiplexing. This prevents unintended side effects, such as eagerly initializing DeploymentHandle Routers during probing. A unit test has also been added to verify this behavior. The reviewer suggests a further improvement: applying inspect.getattr_static to retrieve __dict__ later in the same function to ensure the entire probing process is completely side-effect-free.
…lize handles _callable_uses_multiplexing() scans a replica instance's attributes to detect multiplexing wired up dynamically in a constructor. It read the marker with getattr(), which is not side-effect free: DeploymentHandle.__getattr__ returns options(method_name=...), and options() eagerly initializes the handle's Router. So probing a handle constructed one. Under direct ingress the scan runs at startup on every ingress replica (replica.py _raise_if_multiplexing_with_direct_ingress), so every ingress replica holding a handle built a Router that would otherwise stay lazy -- each carrying a controller long-poll subscription and a periodic metric report stream. The cost scales with replica count. On the 4096-replica controller benchmark (HAProxy on), comparing against the immediate parent commit, this raised handle metric report ingest delay from 383ms to 7275ms, deployment_state_update from 0.117s to 0.242s, and controller asyncio tasks from 9302 to 12182. Reading the marker statically restores it to 827ms / 0.151s / 10004. inspect.getattr_static never invokes __getattr__ or __getattribute__, and reads the marker the decorator sets directly on the wrapper, so detection is unchanged. The existing 'is True' identity check is kept. Adds a regression test asserting the scan never invokes __getattr__ at all. The existing test only covers false positives, whose fake handle has no side effect and so cannot catch this. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
82faa99 to
93c3873
Compare
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: john.taylor <john.taylor@anyscale.com>
abrarsheikh
left a comment
There was a problem hiding this comment.
why does getattr cause options to be called?
Accept added delay and close PR? Or merge PR and fix benchmark to send requests to the replicas? |
Follow on to PR #64045
Why
_callable_uses_multiplexing()probed instance attributes withgetattr(), which on aDeploymentHandletriggers__getattr__→options()→ eager Router initialization.Under direct ingress this scan runs at startup on every ingress replica, so each one
holding a handle built a Router that would otherwise stay lazy — each adding a
controller long-poll subscription and metric-report stream. Cost scales with replicas.
What
Both probes in the scan are now side-effect free:
inspect.getattr_static(), which never invokes__getattr__.__dict__is read withobject.__getattribute__()rather thangetattr(obj, "__dict__", {}), which falls back to__getattr__on a__slots__class.
getattr_staticis not usable here — for__dict__it returns the type'sgetset_descriptorrather than the instance mapping.Detection is unchanged. Adds a regression test asserting the scan never invokes
__getattr__, covering both a handle-holding callable and a__slots__callable; theexisting test only covers false positives, so it cannot catch either.
Test
4096-replica controller benchmark, HAProxy on, vs the immediate parent commit (n=1 per
arm): handle metric report delay 7275ms → 827ms,
deployment_state_update0.242s → 0.151s, controller asyncio tasks 12182 → 10004.