sdk/python: a peer that is gone costs connect one wait, not one per router - #546
Merged
Merged
Conversation
The session refreshes the credential in one worker thread and pulls from the control plane, which may refresh as well, in another; both persist the result with the same temporary file name. On the testnet the loser found its temporary file already renamed by the winner: credential refresh failed, retrying in 30s: [Errno 2] No such file or directory: 'identity.key.tmp' -> 'identity.key' The other outcome is worse: the control plane redeems only the last biscuit it issued, so the loser presents a spent one and the refresh fails until the next attempt. A lock on the member serializes refresh, the pull and save; the fake control plane in the tests now redeems only its last biscuit, as the real one does, and eight refreshes at once all land.
…outer After a rollout the routers' tables still name the pods it replaced, and the sam-sdk-python probe on the testnet ran out its 120s budget on the first provider it tried, printing nothing. connect() walked the relayed paths in turn: a relay whose destination is gone answers a CONNECT only after its own 30s timeout, and a dead provider was tried through every router twice, once as an admitted router and once more as the relay the DHT named for it. py-libp2p's limits on a relayed connection's upgrade add another half minute or more when the relay accepted the circuit and the far end never speaks. The routers of each step are now dialed at once, as join() and the DHT walk already do, and the first circuit that opens ends the others; the DHT step skips relays that are admitted routers, which the first step dialed. A relayed dial is held to DIAL_TIMEOUT as a direct one is, and a circuit given up on is reset so the relay drops it too. The JS SDK dials every address of a peer at once already. A unit test pins the race with three routers that all wait on a dead destination, and one that opens. The SDK mesh integration test gains a relay that accepts every circuit and forwards nothing: every SDK member gives up within its dial timeout and the relay sees each circuit let go.
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces concurrency improvements and timeout boundaries to the Python SDK and integration tests. Specifically, it adds thread-safety locks to credential refresh and control plane synchronization in mesh.py, implements concurrent dialing of routers in session.py to prevent sequential timeouts, and bounds relayed dials with a timeout in relay.py. It also adds corresponding unit and integration tests to verify these changes under concurrent and stalled conditions. There are no review comments to address, and the changes look solid.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
sam-sdk-pythoncold-path probe failed on the bananas deploy of c916264 (and in four scheduled runs before it, on the previous image), always at thenode-mcpstage ashung for 120swith1 attempts, while the JS probe passed. This is what the Python SDK did with the time.After a rollout the routers' tables still name the pods it replaced.
connect()walked the relayed paths in turn: a relay whose destination is gone answers aCONNECTonly after its own 30s timeout (ConnectTimeoutin go-libp2p's relay), and a dead provider was tried through every router twice, once as an admitted router and once more as the relay the DHT named for it. py-libp2p's own limits on a relayed connection's upgrade add another half minute or more when the relay accepted the circuit and the far end never speaks. The first stale provider alone used up the probe's budget.session.py: the routers of each step are dialed at once, asjoin()and the DHT walk already do, and the first circuit that opens ends the others. The DHT step skips relays that are admitted routers, which the first step dialed. The JS SDK dials every address of a peer at once already.relay.py: a relayed dial is held toDIAL_TIMEOUTas a direct one is, and a circuit given up on (timed out, failed, or cancelled because another path won) is reset so the relay drops it too.mesh.py(first commit): the refresh loop and the control plane pull run in separate worker threads and both persist the credential with the same temporary file name; one probe run logged[Errno 2] No such file or directory: 'identity.key.tmp' -> 'identity.key'. The control plane also redeems only the last biscuit it issued, so two refreshes at once leave the loser with a spent one. A lock on the member serializes refresh, the pull and save.Tests:
test_session.py: three routers that all wait on a dead destination costconnect()one wait; one that opens ends the others' attempts.test_relay.py(new): a relay that answersCONNECTwith OK and forwards nothing; the dial ends atDIAL_TIMEOUTand the relay sees the circuit let go. Hangs past 20s on the previous code.test_mesh.py: the fake control plane redeems only its last biscuit, as the real one does; eight concurrent refreshes all land. Seven fail withbiscuit already redeemedon the previous code.TestNativeSDKsMeshgains astalled-relaystep: a Go relay that accepts every circuit and forwards nothing, dialed by every SDK member at once. Each gives up within its dial timeout and the relay counts every circuit let go. The Python member exceeds the 20s budget on the previous code.make sdk-python(96 passed),TestNativeSDKsMeshandTestNativeSDKsAcrossRouterspass with both SDK toolchains present.