fix(netwatch): BSD rebind socket on errors #2913

dignifiedquire · 2024-11-08T09:02:02Z

also uses AbortOnDropHandle to better cleanup tasks

github-actions · 2024-11-08T09:05:05Z

Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2913/docs/iroh/

Last updated: 2024-11-12T21:34:12Z

github-actions · 2024-11-08T09:15:26Z

Netsim report & logs for this PR have been generated and is available at: LOGS
This report will remain available for 3 days.

Last updated for commit: a0561b3

flub · 2024-11-08T10:15:53Z

net-tools/netwatch/src/netmon.rs

+    #[allow(dead_code)]
+    handle: AbortOnDropHandle<()>,


Suggested change

#[allow(dead_code)]

handle: AbortOnDropHandle<()>,

_handle: AbortOnDropHandle<()>,

?

it's actually used, which is why I don't want to use _handle

then it doesn't need #[allow(dead_code)]?

but rust is too stupid..it gets constructed and it has drop semantics.. but the compiler doesn't understand that as usage

Or are we having a philosophical disagreement on what unused means?

My interpretation is the opposite in that case, to me it is definitely not dead code. _thing does not mean something is unused, let _guard = thing(); is an entirely common pattern, this is just the same but in a struct.

you are right, will change

flub · 2024-11-08T10:18:53Z

net-tools/netwatch/src/netmon.rs

-            actor.run().await;
+            if let Err(err) = actor.run().await {
+                warn!("netmon actor died: {:?}", err);
+            }


This is essentially a supervisor. The problem with this version is that you get a single WARN in the logfile and things just carry on. I wonder if maybe it might as well restart the actor - maybe with a small delay. That is still not proper supervision - if it keeps dying we should bring down the entire Endpoint - but it's a start and we'll flood the logs with WARN messages which could be a bit more obvious.

generally yes, but currently there is no error case where this happens (and it is not super easy to restart the actor currently)

flub · 2024-11-08T10:23:09Z

net-tools/netwatch/src/netmon/actor.rs

+                Some(event) = self.mon_receiver.recv() => {
+                    match event {
+                        NetworkMessage::Change => {


Some(NetworkMessage::Change) = self.mon_receiver.recv() might be a nicer way to write this?

I also noticed that NetworkMessage::Change is marked with #[allow(dead_code)]. This seems suspicious, can we remove that marker as well?

This seems suspicious, can we remove that marker as well?

no because this message is not used on android, and I didn't want to start adding cfgs to lint hints

Some(NetworkMessage::Change) = self.mon_receiver.recv() might be a nicer way to write this?

maybe, we usually always put the match inside, so I wrote it like that for consistency

see issues with tokio::select, I cleaned this up to not use an elsebranch

flub · 2024-11-08T10:23:51Z

net-tools/netwatch/src/netmon/actor.rs

@@ -102,7 +101,7 @@ impl Actor {
        self.actor_sender.clone()
    }

-    pub(super) async fn run(mut self) {
+    pub(super) async fn run(mut self) -> Result<()> {


I'm confused, the signature is changed but there's not a single error return?

I had a version where it propagated an error. and then I left this change in, can revert, no strong feelings

yeah, let's revert as then the supervisor discussion goes away as well :)

flub · 2024-11-08T10:25:01Z

net-tools/netwatch/src/netmon/bsd.rs

+    #[allow(dead_code)]
+    handle: AbortOnDropHandle<()>,


Suggested change

#[allow(dead_code)]

handle: AbortOnDropHandle<()>,

_handle: AbortOnDropHandle<()>,

?

I'm not a fan of #[allow(dead_code)] as it easily stays around too long. The _ is much easier to keep track of as the compiler will tell you.

flub · 2024-11-08T10:31:34Z

net-tools/netwatch/src/netmon/bsd.rs

@@ -52,12 +55,25 @@ impl RouteMonitor {
                    }
                    Err(err) => {
                        warn!("AF_ROUTE: error reading: {:?}", err);
+                        // recreate socket, as it is likely in an invalid state


Does this need a small delay in case this is hot-looping?

unclear, the reported error doesn't because the socket is already dead

also uses AbortOnDropHandle to better cleanup tasks Closes #2909

also uses `AbortOnDropHandle` to better cleanup tasks Closes #2909 --------- Co-authored-by: Divma <[email protected]>

## Description With #2913, the netmon actor properly exits when the channel sender is dropped, but the android implementation never kept the sender around. The magicsock actor would then error when trying to subscribe to the network monitor, and the `Endpoint` would be unusable. ## Change checklist - [x] Self-review. - [ ] Documentation updates following the [style guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text), if relevant. - [ ] Tests if relevant. - [ ] All breaking changes documented.

dignifiedquire requested a review from flub November 8, 2024 09:02

flub reviewed Nov 8, 2024

View reviewed changes

flub approved these changes Nov 8, 2024

View reviewed changes

dignifiedquire force-pushed the fix-netmon-bsd-socket-error branch from 270457c to ec61b9b Compare November 12, 2024 16:40

dignifiedquire added this to the v0.29.0 milestone Nov 12, 2024

dignifiedquire added 3 commits November 12, 2024 21:04

fix(netwatch): BSD rebind socket on errors

9fe5207

also uses AbortOnDropHandle to better cleanup tasks Closes #2909

fixup

ac556bf

apply pr review

e3c3455

dignifiedquire force-pushed the fix-netmon-bsd-socket-error branch from ec61b9b to e3c3455 Compare November 12, 2024 20:04

dignifiedquire enabled auto-merge November 12, 2024 20:04

Merge branch 'main' into fix-netmon-bsd-socket-error

db25d91

dignifiedquire added this pull request to the merge queue Nov 12, 2024

Merged via the queue into main with commit c451750 Nov 12, 2024
26 of 27 checks passed

PaulOlteanu mentioned this pull request Nov 13, 2024

fix(netwatch): hold on to netmon sender reference in android #2923

Merged

4 tasks

matheus23 pushed a commit that referenced this pull request Nov 14, 2024

fix(netwatch): BSD rebind socket on errors (#2913)

7894069

also uses `AbortOnDropHandle` to better cleanup tasks Closes #2909 --------- Co-authored-by: Divma <[email protected]>

dignifiedquire deleted the fix-netmon-bsd-socket-error branch November 28, 2024 10:10

	#[allow(dead_code)]
	handle: AbortOnDropHandle<()>,
	_handle: AbortOnDropHandle<()>,

fix(netwatch): BSD rebind socket on errors #2913

fix(netwatch): BSD rebind socket on errors #2913

Uh oh!

Conversation

dignifiedquire commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dignifiedquire commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading

github-actions bot commented Nov 8, 2024 •

edited

Loading