-
Notifications
You must be signed in to change notification settings - Fork 37
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
Conversation
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 Could you try to use |
Good point about the tests! I forgot about that here because I tested it in the I'll work on adding tests and de-coupling |
I managed to completely decouple tokio from the replication stuff using 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. |
I moved the 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 An aside: the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
.github/workflows/ci.yml
Outdated
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Also fix some small lints
To make replication feature runtime agnostic
This was needed for tests. This is cheap and I imagin end users would want this too.
There was a problem hiding this 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.
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 aimpl CoreMethods
which will work for bothHypercore
andArc<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 requirestokio
because it defines aSharedCore
type (that usestokio::sync::Mutex
) which is intended to be the de-facto implementation for downstream libraries to use when they want aHypercore
that can have multiple owners. I use this downstream in thereplicator
crate.In theory, the dependence on
tokio
could be removed, by movingSharedCore
to it's own crate. And the other usage oftokio::sync::broacast
could be replaced withasync_broadcast
which is async runtime agnostic. However this seemed like it could be more overhead than it is worth.