Repository navigation
agent: restart the retry loops when the network changes - #1983
Closed
Narasimha-sc wants to merge 10 commits into
Closed
Narasimha-sc wants to merge 10 commits into
Narasimha-sc wants to merge 10 commits into
Conversation
… data takes to fail TCP keep-alive probes are only sent while there is no unacknowledged data, so a connection that died with data in flight is only reported as failed when the OS gives up retransmitting - ~15 minutes on Linux and Android. When no network change is reported to the agent (a silent route change, a client without a network observer) that is how long delivery keeps failing on the dead client. KeepAliveOpts gets unackedDataTimeout: when it is set, the sockets that set TCP keep-alive also set TCP_USER_TIMEOUT (TCP_RXT_CONNDROPTIME on Darwin, not set on Windows) to the keep-alive detection time, so that such a connection fails after the same time as an idle one. On Linux this timeout also bounds the zero-window persist timer, so a server that stops reading for that long then fails the connection. The option is off unless the client sets it, so that it can be enabled by the users who test it before it becomes the default. It is a Maybe to keep the clients that do not send it working. As it is derived from the keep-alive options where they are set, the two detection times cannot diverge and lowering the keep-alive settings lowers both. It is removed for the connections via SOCKS proxy - the socket is then connected to the proxy, its acknowledgements only mean that the proxy buffered the data, and a slow circuit should not fail the connection.
After the network changes (e.g. the IP address changed after internet loss) the connections to the servers are dead, but the OS reports it only after TCP retransmissions time out - ~15 minutes on Linux/Android while the agent keeps writing to the dead socket. Until then message delivery, subscriptions and file transfers keep failing on the same client. setUserNetworkInfo on every online network event (the clients report one when the network type changes and when it goes offline to online): - closes the connected SMP and NTF clients and fails their pending requests, so that the workers that resume do not wait for the responses that will not arrive - the requests are failed only once the client's threads terminated, which the client now records, as its session var is also removed when a resubscription fails (each client type bounded by 1 second; the requests of a client that did not terminate in this time are left to time out as before); - discards the errors of the clients that failed to connect, cached for persistErrorInterval, and removes a client that is still connecting, in the transaction that reads it, so that the resumed workers create a new client instead of waiting for a connection made via the previous network - that client is closed by newProtocolClient when it finds it is no longer in the map; - closes the XFTP clients asynchronously, removing them from the map. A client that is no longer the one in the map is rejected in the same transaction that would publish it, so that the requests made on it do not wait for the timeout, and it is closed in a forked thread, as closing an XFTP client waits for its thread to terminate; it sets its session id while its var is the one in the map and updates its service only if it was. The clients are closed in a forked thread, so that the API does not block, and the closes of an event that a newer one superseded are skipped. When the agent was offline the reported network info is applied when the clients are closed, so that the workers waiting for the network do not resume before then; when it was online it is applied immediately. As the clients are closed, the network change now results in DISCONNECT and DOWN events followed by CONNECT and UP, as any other reconnection. The retry loops are not restarted, so a worker already waiting out its retry interval resumes only when it expires.
… reachable Closing the clients on a network change makes a client that is no longer the one in the session map a routine occurrence rather than a rare one, which makes these pre-existing races reachable in normal use. - setSubsPending removes the subscriptions of a transport session only when the session id matches. A client that is no longer the one in the map never set its session id, and when the session mode stopped matching the transport session its disconnection removed the subscriptions of the client that replaced it without making them pending, so they were neither subscribed nor scheduled and nothing corrected it until the app restarted. - smpClientDisconnected removes the sessions to the proxied relays only when no other client replaced this one, as those sessions are then the sessions of the client that replaced it. - updateClientService reads the stored service id in the transaction that writes it. The id read before the handshake can be stale, as the transaction of a client that is no longer current can commit after the one of the client that replaced it, and removeRcvServiceAssocs would then run against a service id that changed since, removing the associations of the current client.
Closing the clients when the network changes makes the workers fail their current attempt, but a worker already waiting out its retry interval resumes only when it expires - up to the maximum interval, so delivery and subscriptions can take minutes after the network is usable again. The network epoch is incremented when the clients are closed, and the agent retry loops restart from the loop's initial interval when it changes - for delivery and XFTP workers resumed with persisted retry state that is the persisted interval (withRetryEpoch). Message delivery restarts the fast interval only, the slow interval for QUOTA errors is unchanged; the subscription worker keeps its foreground and online resets; retry attempt counters are not reset. A loop that was waiting for the network sees the epoch change after its first attempt, so if that attempt fails it retries once more without delay. withRetryWait2 now takes the value to sample before the action and a wait that receives the retry interval mode and returns whether the interval restarts, so one loop serves the plain, the lock and the epoch variants - the slow interval retries in STM and falls through to the plain wait, so it is unchanged. withRetryEpoch2 is withRetryLock2 without the lock, for the command worker, which retries with two intervals.
… data takes to fail TCP keep-alive probes are only sent while there is no unacknowledged data, so a connection that died with data in flight is only reported as failed when the OS gives up retransmitting - ~15 minutes on Linux and Android. When no network change is reported to the agent (a silent route change, a client without a network observer) that is how long delivery keeps failing on the dead client. KeepAliveOpts gets unackedDataTimeout: when it is set, the sockets that set TCP keep-alive also set TCP_USER_TIMEOUT (TCP_RXT_CONNDROPTIME on Darwin, not set on Windows) to the keep-alive detection time, so that such a connection fails after the same time as an idle one. On Linux this timeout also bounds the zero-window persist timer, so a server that stops reading for that long then fails the connection. The option is off unless the client sets it, so that it can be enabled by the users who test it before it becomes the default. It is a Maybe to keep the clients that do not send it working. As it is derived from the keep-alive options where they are set, the two detection times cannot diverge and lowering the keep-alive settings lowers both. It is removed for the connections via SOCKS proxy - the socket is then connected to the proxy, its acknowledgements only mean that the proxy buffered the data, and a slow circuit should not fail the connection.
# Conflicts: # src/Simplex/Messaging/Client.hs # tests/AgentTests/FunctionalAPITests.hs
# Conflicts: # src/Simplex/Messaging/Agent.hs # tests/AgentTests/FunctionalAPITests.hs
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.
Closing the clients when the network changes makes the workers fail their current attempt, but a worker already waiting out its retry interval resumes only when it expires - up to the maximum interval, so delivery and subscriptions can take minutes after the network is usable again. This matters most when the IP changes without an offline event (a roam to another network, or a lease renewed while still associated): the workers are not parked on
waitForUserNetwork, they are in backoff, so closing the clients does not wake them.The network epoch is incremented when the clients are closed, and the agent retry loops restart from the loop's initial interval when it changes - for delivery and XFTP workers resumed with persisted retry state that is the persisted interval (
withRetryEpoch).With this, a reported IP change recovers in 3-4 s in the harness, against 808 s before the stack.
135 of the added lines are
RetryIntervalTestscovering the epoch, including mixed slow/fast intervals and an epoch change during the action.Rebased onto master
masterhad refactoredwithRetryLock2into a generalised internalwithRetryWait2, and moved the command worker from a single retry interval to two (loop RISlow/loop RIFast). Rather than duplicate master's loop,withRetryWait2is generalised once more: it takes the value to sample before the action - sampling before matters, so that an epoch change during the action also restarts the interval - and awaitthat receives theRetryIntervalModeand returns whether the interval restarts. One loop then serves the plain, lock and epoch variants.RISlowretries in STM and falls through to the plain wait, so the slow quota interval is byte-for-byte unchanged in behaviour.withRetryEpoch2is added for the command worker:withRetryLock2without the lock. It is covered by master's own parameterised 2-mode tests, which now run against all three variants.Retry intervalis 15 examples, 0 failures (was 11 before the rebase, as master's parameterisation adds cases);user network infois 5/5.Stack
Reducing reconnection delay after a network change. Each PR is based on the previous one and can be merged on its own once the ones below it are in.
mastermasterMoved from #1929 (head branch moved to fork).
Depends on #1928: this branch includes its commits; the diff shrinks once that PR merges.