diameterService: iterate over a snapshot of activePeers - #337
Conversation
self.activePeers is shared between the asyncio coroutines of the diameter service. handleOutboundDwr(), handleActiveDiameterPeers() (the loop that marshals the peers into Redis) and logActivePeers() iterate over it with an await inside the loop body. While they are suspended, handleConnection() adds or updates peers and handleActiveDiameterPeers() deletes stale ones, which raises RuntimeError: dictionary changed size during iteration The loops' "except Exception" logs the error and aborts the current pass, so the remaining peers get no DWR, no connection-state metric and no Redis peer-table refresh until the next pass. Iterate over list()/dict() snapshots of self.activePeers instead. The Redis marshal loop takes its own snapshot after the stale peers have been pruned, so that removed peers are not written back to Redis. The loops in lib/diameter.py named in the issue iterate over a fresh dict returned by getAllHashData() and are not affected. Fixes nickvsnetworking#310
Exercise handleOutboundDwr(), handleActiveDiameterPeers() and logActivePeers() with a peer being added to or removed from self.activePeers while the loop awaits, as happens when peers connect or disconnect (nickvsnetworking#310). Also check that the pruner removes duplicate and stale peers and does not write pruned peers back to Redis. Add the file to the ruff include list.
Takuto88
left a comment
Takuto88
left a comment
There was a problem hiding this comment.
LGTM! Thank you very much for your contribution!
I've verified that this is a problem on the current master branch thanks to your test, which does reproduce the issue without your patch in 3298fca.
The only thing that I see a bit critically is that the test relies on log statements as the except Exception: await logTool.logAsync(... 'Exception: {traceback}') simply prevents the exception to reach the test. So the log message part "Exception: " becomes load bearing.
This is still okay in my opinion as this entire mechanism how diamteter works could use a major overhaul in any case and until then, this is a good enough solution. If someone changes the log message there or refactors the code, the test will just become red and we can deal with that then.
2 participants
Fixes #310
What raises it.
services/diameterService.pyiterates the sharedself.activePeersdict in three coroutines with anawaitinside the loop body:handleOutboundDwr(awaitssendMessage), the Redis-marshal loop inhandleActiveDiameterPeers(awaitssetHashValue) andlogActivePeers(awaitssendMetric). While the loop is suspended,handleConnectioninserts a new peer and the pruner inhandleActiveDiameterPeersdeletes stale ones, so the dict changes size under the iteration. Theexcept Exceptionin each loop logs the error and abandons the rest of the pass, so the remaining peers get no DWR, no metric and no Redis refresh for that round.The loops the issue points at in
lib/diameter.py(getDraPeers/getConnectedPeersByType/getPeerByHostname) iterate a local dict returned fresh bygetAllHashData()and cannot raise this, so they are left alone.Fix. Iterate over a snapshot at those sites (
list(self.activePeers.items())/dict(self.activePeers)). The marshal loop takes a fresh snapshot after the pruner has run, so peers deleted a moment earlier are not written back to Redis. No locks, no behaviour change: a peer that connects mid-pass is picked up on the next pass, which is the existing cadence.Tests.
tests/test_diameter_service_peers.pydrives each of the three loops with a stubbed Redis/logTool whose awaited call adds or removes a peer mid-iteration. On master all five cases fail with the exactRuntimeError: dictionary changed size during iterationat the three loop lines; with the fix they pass. The file is added to the ruff include list and carries the SPDX headertest_license_headers.pychecks. A[Unreleased]entry is added toCHANGELOG.mdasdocs/release.mddescribes.ruff checkandruff format --checkare clean;pytest tests/test_diameter_service_peers.py tests/test_license_headers.pypasses (the full suite needs the CI services).