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

Add events needed for replication #142

Merged
merged 24 commits into from
Oct 25, 2024
Merged

Conversation

cowlicks
Copy link
Contributor

This adds some events to Hypercore that are needed for replication to work.

It also defines some traits that can get used in place of a concrete Hypercore. So downstream libraries can just consume a impl CoreMethods which will work for both Hypercore and Arc<Mutex<Hypercore>>.

I also fixed some TODO's and small lints I came across in the docs.

One thing that may be controversial:
The replication feature requires tokio because it defines a SharedCore type (that uses tokio::sync::Mutex) which is intended to be the de-facto implementation for downstream libraries to use when they want a Hypercore that can have multiple owners. I use this downstream in the replicator crate.

In theory, the dependence on tokio could be removed, by moving SharedCore to it's own crate. And the other usage of tokio::sync::broacast could be replaced with async_broadcast which is async runtime agnostic. However this seemed like it could be more overhead than it is worth.

src/core.rs Outdated Show resolved Hide resolved
@ttiurani
Copy link
Member

Thanks for the PR, overall looks reasonable!

However, there are no tests for it here now at all, and that makes it fragile to maintain. Would you consider adding tests?

Wrt the async_std support, this is quite a big decision. After so much effort in having multi-async support, just casually dropping it is kinda a massive decision? Personally I'd either drop async_std everywhere consistently (random-access-disk and hypercore-protocol-rs) and make it all tokio-only, or then put in the effort to make it work in both.

Even though async-std might get deprecated in the coming years, I can see hypercore being useful in some smol environments, and swapping those async runtimes is doable if we don't do a tokio-only features here.

Could you try to use async_broadcast to see what that would look like?

@cowlicks
Copy link
Contributor Author

Good point about the tests! I forgot about that here because I tested it in the replicator crate. However it'd be good to have tests here too.

I'll work on adding tests and de-coupling tokio & replication here.

@cowlicks
Copy link
Contributor Author

cowlicks commented Oct 22, 2024

I managed to completely decouple tokio from the replication stuff using smol's crates which are runtime agnostic (It may be possible to do the same thing within the random-access-* crates which would let us drop the tokio & async-std features completely so that we could be completely runtime agnostic). It turned out I did not need tokio for SharedCore so there was no need to remove it.

I also added a few tests. There are probably more needed but I am done for the day. I also need to enable the tests in the CI.

@cowlicks
Copy link
Contributor Author

I moved the SharedCore behind it's own feature (shared-core = ["replication", "dep:async-lock"]).

I see we test, almost every combination of features, and instead of multiplying the number of tests runs by x12 (for the two new features x (windows, mac, linux)) I added just one cargo test --no-default-features --features tokio,shared-core to each target. These are added before the existing & failing js_interop_tests so you can see they pass in the CI.

An aside: the async-std and tokio features only effect dependencies. So testing with both of these seems like we are testing implementation details of our own dependencies. It may be better to leave that to those dependencies to test themselves.

Copy link
Member

@RangerMauve RangerMauve left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -38,6 +38,7 @@ jobs:
cargo check --no-default-features --features async-std
cargo check --no-default-features --features async-std,sparse
cargo check --no-default-features --features async-std,sparse,cache
cargo test --no-default-features --features tokio,shared-core
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you aren't adding also async-std replication tests here too, instead of these tokio-only tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added the async-std tests.

Copy link
Member

@ttiurani ttiurani left a comment

Choose a reason for hiding this comment

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

Fantastic work! Looks perfect now.

Thank you for fixing also TODOs along the way.

@ttiurani ttiurani merged commit c146362 into datrs:master Oct 25, 2024
6 checks passed
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.

3 participants