Skip to content

router: ensure the router proxy loop is broken out of on shutdown#413

Merged
wusyong merged 1 commit intomainfrom
fix_router_shutdown
Aug 28, 2025
Merged

router: ensure the router proxy loop is broken out of on shutdown#413
wusyong merged 1 commit intomainfrom
fix_router_shutdown

Conversation

@gterzian
Copy link
Copy Markdown
Member

@gterzian gterzian commented Aug 27, 2025

The current shutdown does a break, but that only breaks out of the inner event-handling loop, so the thread doesn't actually shut down. This replaces it with a return, and also joins on the thread handle for good measure. Includes unrelated changes from an fmt.

Testing: added a unit test.

…read handle

Signed-off-by: gterzian <2792687+gterzian@users.noreply.github.com>
@gterzian
Copy link
Copy Markdown
Member Author

Relates to servo/servo#30849

@gterzian gterzian requested a review from Taym95 August 27, 2025 10:09
Copy link
Copy Markdown
Member

@mrobinson mrobinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

Comment thread src/router.rs
.take()
.expect("Should have a join handle at shutdown")
.join()
.expect("Failed to join on the router proxy thread");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we can avoid panicking here:

if let Err(err) = handle.join() {
    error!("Router thread panicked during shutdown: {:?}", err);
}

@wusyong wusyong added this pull request to the merge queue Aug 28, 2025
Merged via the queue into main with commit c8d07cf Aug 28, 2025
16 checks passed
@wusyong wusyong deleted the fix_router_shutdown branch August 28, 2025 04:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants