datrs / hypercore

Secure, distributed, append-only log
https://docs.rs/hypercore
Apache License 2.0
334 stars 37 forks source link

Add events needed for replication #142

Closed cowlicks closed 4 weeks ago

cowlicks commented 1 month ago

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.

ttiurani commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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.