Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dgw): polish scanner, fix issues #715

Merged
merged 17 commits into from
Feb 28, 2024
Merged

fix(dgw): polish scanner, fix issues #715

merged 17 commits into from
Feb 28, 2024

Conversation

irvingoujAtDevolution
Copy link
Contributor

@irvingoujAtDevolution irvingoujAtDevolution commented Feb 21, 2024

fixes the folloing issue

  1. this should stop error message from mdns_sd
  2. scanner should be stopped correctly now, let me know if on your side @CBenoit

Issue: DGW-131

crates/network-scanner/src/mdns.rs Outdated Show resolved Hide resolved
crates/network-scanner/src/mdns.rs Outdated Show resolved Hide resolved
crates/network-scanner/src/mdns.rs Show resolved Hide resolved
@CBenoit
Copy link
Member

CBenoit commented Feb 22, 2024

It’s now closing but not in a clean way. Everything should be good if you add this line:

index 440d2f5..4b58f17 100644
--- a/devolutions-gateway/src/api/net.rs
+++ b/devolutions-gateway/src/api/net.rs
@@ -74,6 +74,8 @@ pub async fn handle_network_scan(
         info!("Network scan finished");

         stream.stop();
+
+        let _ = websocket.close().await;
     });

     Ok(res)

@irvingoujAtDevolution
Copy link
Contributor Author

It’s now closing but not in a clean way. Everything should be good if you add this line:

index 440d2f5..4b58f17 100644
--- a/devolutions-gateway/src/api/net.rs
+++ b/devolutions-gateway/src/api/net.rs
@@ -74,6 +74,8 @@ pub async fn handle_network_scan(
         info!("Network scan finished");

         stream.stop();
+
+        let _ = websocket.close().await;
     });

     Ok(res)

I added this at where the stream receives none

@CBenoit
Copy link
Member

CBenoit commented Feb 23, 2024

I added this at where the stream receives none

Why not just keep this at the end?

stream.stop();

let _ = websocket.close().await;

This looks less error-prone to me. Is it causing any problem?

@irvingoujAtDevolution
Copy link
Contributor Author

irvingoujAtDevolution commented Feb 23, 2024

I added this at where the stream receives none

Why not just keep this at the end?

stream.stop();

let _ = websocket.close().await;

This looks less error-prone to me. Is it causing any problem?

Not really, it‘s just the logic to me. looks more precise, if the select ends because of the second branch, where the client sends close message, it feels not right to send back a message on a closed connection. Though, it practially makes little difference.

@CBenoit CBenoit enabled auto-merge (squash) February 28, 2024 14:12
@CBenoit CBenoit merged commit 0c6f644 into master Feb 28, 2024
16 checks passed
@CBenoit CBenoit deleted the Network-Scan-fixes branch February 28, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants