feat(rust): add timer to close secure channel #5200
Conversation
implementations/rust/ockam/ockam_identity/src/secure_channel/handshake/handshake_worker.rs
Outdated
Show resolved
Hide resolved
implementations/rust/ockam/ockam_identity/src/secure_channels/secure_channels.rs
Outdated
Show resolved
Hide resolved
implementations/rust/ockam/ockam_identity/src/secure_channel/handshake/handshake_worker.rs
Outdated
Show resolved
Hide resolved
|
I like it! The direction is excellent! The only note I have is that using a boolean is somewhat fragile because you don't have any guarantee about when a task is actually run. For example, if some piece of the code misbehaves and blocks an asynchronous thread, every task that is using that thread will be suspended until the piece of code returns. In this case, the secure channel may be idle for the duration of the suspension (which could be minutes) and not detect it. The most common solution would be to store the current timestamp (less performant) instead of storing the boolean. But being a simple timeout, even if we are not super precise, I think it's ok, the goal is to clean up resources, and it's doing that. |
|
Hi @davide-baldo ! |
etorreborre
force-pushed
the
Alef-gabriel/timer-to-close-secure-channel
branch
from
8848ca3 to
0c9d3b7
Compare
since this is just a boolean, not the timeout itself Also, moved the code resetting the boolean to a better place
There was a problem hiding this comment.
Nice approach. Left few comments. Also, could you also please clone the idle flag to the Encryptor worker? Because currently using the encryption side of the Secure Channel doesn't reset the flag, therefore if we only use one-side communtication, the Secure Channel will be shut down soon :)
| @@ -1106,7 +1106,7 @@ async fn should_stop_encryptor__and__decryptor__in__secure_channel( | |||
| let local_info = IdentitySecureChannelLocalInfo::find_info(msg.local_message())?; | |||
| assert_eq!(local_info.their_identity_id(), alice.identifier()); | |||
| assert_eq!("Hello, Bob!", msg.body()); | |||
|
|
|||
| dbg!(ctx.list_workers().await?); | |||
There was a problem hiding this comment.
Do you think it makes sense to remove dbg! after the test is passing?
| let bob = identities_creation.create_identity().await?; | ||
| let alice = identities_creation.create_identity().await?; | ||
|
|
||
| let bob_trust_policy = TrustIdentifierPolicy::new(alice.identifier()); |
There was a problem hiding this comment.
This is not a needed detail for that test. Might just use SecureChannelListenerOptions::new(). But it's also OK as it is
|
|
||
| let msg = child_ctx.receive::<String>().await?; | ||
|
|
||
| let local_info = IdentitySecureChannelLocalInfo::find_info(msg.local_message())?; |
There was a problem hiding this comment.
I think these 2 lines are not needed here
|
|
||
| let mut additional_workers = ctx.list_workers().await?; | ||
| additional_workers.retain(|w| !initial_workers.contains(w)); | ||
| let works_count = additional_workers.len(); |
There was a problem hiding this comment.
Could we please, instead of relying on the number of workers, instead check if the specific encryptors and decryptors are on the list of workers? I think there are potentially a lot of workers on the system that can start/stop even though we don't do anything, which can make this test incorrect in the future.
There was a problem hiding this comment.
That is also potentially the reason why should_stop_encryptor__and__decryptor__per__timeout is failing on the CI
| // start a background thread to observe the connection activity | ||
| // If a message is received before maximum_idle_time has passed then the connection is | ||
| // active. Otherwise, we close the secure channel | ||
| spawn(async move { |
There was a problem hiding this comment.
That will shut down the channel in the range of [maximum_idle_time, 2 * maximum_idle_time) from the last message. Not quite precise, but it should be fine. Could you please mention that detail in the doc-comment?
Also, mentioning that Secure Channels are shut down due to inactivity is worth mentioning somewhere in the doc-comments of the public API. Could you please add that?
| self_clone | ||
| .stop_secure_channel(&ctx_clone, &addr) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
Let's please handle the error gracefully, we generally want to avoid panics
| @@ -99,6 +98,7 @@ impl Worker for IdentityChannelListener { | |||
| self.options.trust_context.clone(), | |||
| None, | |||
| None, | |||
| None, | |||
There was a problem hiding this comment.
What is the reason that we don't use idle flag here?
|
Hey @Alef-gabriel , do you plan to get back to this PR? |
Current behavior
Currently, when to establish a secure channel, two workers are created: decrypt and encrypt. However, when the initiator shuts down its connection without explicitly removing the workers, they remain active on the responder side. This can lead to issues such as performance degradation or memory consumption over time.
Proposed changes
A timer to shut down the workers when a message is not received