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

Expose Shadowsocks fd on Android #6760

Merged
merged 1 commit into from
Sep 10, 2024
Merged

Expose Shadowsocks fd on Android #6760

merged 1 commit into from
Sep 10, 2024

Conversation

dlon
Copy link
Member

@dlon dlon commented Sep 7, 2024

This updates shadowsocks-rust to 1.20.3. Unblocks #6672.


This change is Reviewable

@dlon dlon force-pushed the add-shadowsocks-fd branch from 5017bac to 94b8a29 Compare September 9, 2024 11:30
@dlon dlon marked this pull request as ready for review September 9, 2024 11:31
@dlon dlon requested a review from Serock3 September 9, 2024 11:31
@dlon dlon force-pushed the add-shadowsocks-fd branch from 94b8a29 to 9d09315 Compare September 9, 2024 11:37
Copy link
Contributor

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dlon)


Cargo.lock line 1624 at r1 (raw file):

[[package]]
name = "icu_collections"

Wow, that's a lot of new dependencies from just bumping the version. Oh well.


tunnel-obfuscation/src/shadowsocks.rs line 111 at r1 (raw file):

    #[cfg(target_os = "android")]
    let _ = outbound_fd_tx.send(shadowsocks.as_raw_fd());

It's unfortunate that we need to use a oneshot to send the return value. Could we not run create_shadowsocks_client outside this function and have access to the file descriptor directly?


tunnel-obfuscation/src/shadowsocks.rs line 138 at r1 (raw file):

    client.abort();
    server.abort();

Unrelated to the PR, but this could be simplified to

    tokio::select! {
        _ = shutdown_rx => {
            log::trace!("Stopping shadowsocks obfuscation");
        }
        _result = handle_outgoing(
            shadowsocks.clone(),
            local_udp.clone(),
            wg_addr.clone(),
        ) => log::trace!("Shadowsocks client closed"),
        _result = handle_incoming(shadowsocks, local_udp, wg_addr) => log::trace!("Local UDP client closed"),
    }

Code quote:

    let mut client = tokio::spawn(handle_outgoing(
        shadowsocks.clone(),
        local_udp.clone(),
        wg_addr.clone(),
    ));
    let mut server = tokio::spawn(handle_incoming(shadowsocks, local_udp, wg_addr));

    tokio::select! {
        _ = shutdown_rx => {
            log::trace!("Stopping shadowsocks obfuscation");
        }
        _result = &mut server => log::trace!("Shadowsocks client closed"),
        _result = &mut client => log::trace!("Local UDP client closed"),
    }

    client.abort();
    server.abort();

@dlon dlon force-pushed the add-shadowsocks-fd branch 2 times, most recently from f867df7 to 1f67b96 Compare September 9, 2024 12:25
Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @Serock3)


tunnel-obfuscation/src/shadowsocks.rs line 111 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

It's unfortunate that we need to use a oneshot to send the return value. Could we not run create_shadowsocks_client outside this function and have access to the file descriptor directly?

Done.

@dlon dlon force-pushed the add-shadowsocks-fd branch 2 times, most recently from eda36fc to 8413f76 Compare September 9, 2024 12:34
Copy link
Contributor

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon force-pushed the add-shadowsocks-fd branch 4 times, most recently from 9d8df6a to 3d9e42a Compare September 9, 2024 15:23
@dlon dlon force-pushed the add-shadowsocks-fd branch from 3d9e42a to d090bd0 Compare September 10, 2024 07:17
@dlon dlon merged commit d090bd0 into main Sep 10, 2024
54 of 55 checks passed
@dlon dlon deleted the add-shadowsocks-fd branch September 10, 2024 07:31
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.

2 participants