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

Allow using the crate without tokio #43

Closed
wants to merge 26 commits into from

Conversation

Congyuwang
Copy link
Contributor

@Congyuwang Congyuwang commented Jul 23, 2024

Previously the crate requires tokio. This PR makes tokio optional behind a default feature gate tokio. Now, when using this crate with --no-default, it can be used with other runtimes such as async-std and smol without depending on tokio.

This PR does not affect the old users since tokio is enabled by default.

Closes #42

@Congyuwang Congyuwang marked this pull request as draft July 24, 2024 08:54
@Congyuwang Congyuwang marked this pull request as ready for review July 24, 2024 10:07
@Congyuwang
Copy link
Contributor Author

I've also added tests for no-default-features and also cargo clippy checks.

This PR should not affect existing users. If it does, it is a bug.

@sticnarf
Copy link
Owner

Though this is not a goal of this crate (because it is named after tokio-socks), it's possible to use it under other async runtimes even now.

tokio-util helps you convert tokio IO traits to futures IO traits. Then, you can use connect_with_socket with network types from e.g. smol.

Therefore, I don't want to include the compat layer directly in this crate because it's actually unnecessary and out of the scope of the crate.

Nevertheless, I think it okay to make tokio/net an optional feature. Then, tokio becomes very lightweight with only io-util enabled.

@Congyuwang
Copy link
Contributor Author

Congyuwang commented Jul 24, 2024

For my use case, there are teams that just do not want tokio to appear in their Cargo.lock (maybe just to make things clean).

Making tokio/net optional (which is what I initially started doing) with just a little bit of extra effort, you can remove tokio completely. This is because this crate (tokio-socks) is nicely written.

Also, I believe that the refactor I proposed is actually cleanly organized. Each of the original socks4.rs and socks5.rs are converted into a module with mod.rs, tokio_impl.rs and futures_impl.rs, where the main body is kept in mod.rs. So, although it may seem like a big change set, it actually is not, it is just moving codes around. I think it wouldn't create a very big maintainability issue, but that should be left to your judgement.

I've also added clippy in CI to ensure that the code is clean for different feature sets.

@sticnarf
Copy link
Owner

sticnarf commented Jul 24, 2024

For my use case, there are teams that just do not want tokio to appear in their Cargo.lock (maybe just to make things clean).

I see you're contributing to zed. I think it's not true for their case. Tokio exists in their codebase. They just don't use the tokio runtime (and its network parts of course).

It's quite common to use tokio not for their runtime. For example, it's clean to import tokio only for its synchronization primitives like Mutex or Semaphore.

It should be the similar case for this crate. Tokio with only io-util is not something heavy that cannot be depended on.

@Congyuwang
Copy link
Contributor Author

Congyuwang commented Jul 24, 2024

Okay. What do you think about this idea?

Since I also find feature gates a bit nasty as they have to be additive, what if we don't use features to do this?

We can change the repo into a rust workspace, where the old "tokio-socks" package does its job for tokio and keeps its API unchanged, while we refactor the core functionality to use traits from futures-io (it just strikes me that futures-io and tokio/io are almost drop-in replacement for each other in the most difficult core part of the crate) and place that into another package in the same workspace. Then use Compat to wrap it for tokio-socks crate.

This way we don't have to worry about the nasty feature gates.

What do you think?😁 If you find this promising, I can close this PR and work on it.

Basically so we can publish two crates using the same core code.

@sticnarf
Copy link
Owner

Right... I'm particularly not a fan of mutual exclusive features. It's a huge mess for users.

This is the commit of my scratch code of the solution: eb2e5f7

We can create our own IO trait. And by default, the trait is implemented for tokio IO types to keep backward compatibility. Users can wrap futures-io types with the Compat adaptor to easily satisfy the crate's IO trait.

@Congyuwang
Copy link
Contributor Author

That's a very nice solution! Great! It makes the features no longer conflicting each other.

@Congyuwang Congyuwang closed this Jul 24, 2024
@Congyuwang Congyuwang reopened this Jul 24, 2024
@Congyuwang
Copy link
Contributor Author

I think I can help furnish tests, clippy & formatting by PR to compat branch. If you need this, you can tell me.

@sticnarf
Copy link
Owner

You can continue with my code and that would be great.

@Congyuwang
Copy link
Contributor Author

Okay, let me give it a try. Closing this one.

@Congyuwang Congyuwang closed this Jul 24, 2024
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.

Support futures ecosystem (that is: better support for non-tokio runtimes)
2 participants