(do not merge yet) Erase generic parameter by boxing Storage as trait object

Frando commented 4 years ago

Currently, the Feed struct is generic over T: RandomAccess<Error = Box<dyn std::error::Error + Send + Sync>> + Debug + Send. This means that the generic type has to be carried through all code that works with Feed. This also means it's not currently possible to have e.g. a HashMap that hasFeeds that use differentRandomAccess` storages (e.g. disk and in-memory).

In this (unfinished) PR this generic type argument is removed. Instead, internally the Feed has a boxed DynStorage trait object that has all public methods of the Storage struct (storage module).

Thus, the generic type is erased for upstream uses!

This PR is unfinished. Signatures of the tests have to be changed. Also I didn't think fully think through what parts of this should be exposed as public APIs. Maybe instead of changing all tests (apart from removing the trait argument) we can also rename the DynStorage in this PR to Storage and thus keep the storage API mostly the same, not sure. I think this would need type ascriptions though that's why I went for the public functions to create new storage instances.

@bltavares please feal free to continue here and also push directly to this branch! Not sure when I find the time to get back to this.

Checklist

[x] hypercore compiles
[ ] review how storage is exposed publicly (DynStorage vs Storage vs public functions storage_memory and storage_disk
[ ] tests compile
[ ] tests pass
[ ] tests and/or benchmarks are included
[ ] documentation is changed or added

Context

See #109 for initial motivation

Semver Changes

Major, because the generic type argument is removed. Likely release together with the async change (this PR also is on top of the async branch).

Frando commented 4 years ago

I think it would be even nicer if the generic argument would be removed from the Storage trait alltogether and instead each of the storages (tree, bitfield, data, keypair etc) would instead be boxed trait objects. This would also allow to use different backends for different storages. This is possible in NodeJS hypercore and allows for some nice addons/features. However, I could not find a way to make that work, because:

The RandomAccess trait has a method async fn read_to_writer(&mut self, offset: u64, length: u64, buf: &mut (impl futures_io::AsyncWrite + Send)) -> Result<(), Self::Error>
Because this method has a generic parameter, it cannot be boxed into a trait object

I don't know if there is a clean workaround for this. If there is, this would be my preferred option. If there is not, I propose to go forward with the implementation as in this PR. Also, this PR could be an intermediate step (allowing different storages within the Storage trait wouldn't even be a breaking change necessarily, as this is a internal non-public module).

bltavares commented 4 years ago

@Frando I've rebased against master, introducing all changes that were pending. Try to pull again the branch when you come back.

bltavares commented 4 years ago

@Frando tests are passing, and I ended up exposing storage_memory and storage_disk to be able to create new storages using the Public API.

I have not considered the Public API yet, I've only focused on making it pass.

I will try to remove the extra generic argument as you mentioned. Wish me luck hehe

khodzha commented 4 years ago

why not just box impl futures_io::AsyncWrite + Send?

bltavares commented 4 years ago

I'm documenting some of the discussion from IRC based on the suggestion from @khodzha to use the async traits.

This does allow the RandomAccessStorage to become a trait object and to be boxed. With that, it is possible to write the additional methods on top of AsycnRead, AsyncWrite and AsyncSync, require extra methods, and provide some extension methods and 'utility functions'.

That would be yet another breaking change on random-access-storage, but it would be great to support a wider set of crates using the std methods.

This first attempt is compiler-error-driven and I have no much idea if that is enough for what hypercore needs. It would be great to try this out with a fork of random-access-storage and see how it plays out. That is a big change tho, so be aware.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=f12dcb9ff6262e561a2fa17ab916538e

bltavares commented 4 years ago

I would even wonder if we do need those extra random_access_methods on the trait or if all of those could be provided as extension methods based on the underlying traits, like the read_at and write_at. 🤔

bltavares commented 4 years ago

Maybe random_access_storage could also take sync structs if we convert them to async like smol does for reader and writers: https://docs.rs/smol/0.1.18/smol/fn.reader.html

Frando commented 3 years ago

I rebased this against master today and have forced-push this PR so that CI can run. I had some git rebase foo which makes the history a little ugly. The old branch before the rebase is also pushed to dyn-storage-old (if someone would want to rewrite history better than I did).

I also added a test that adds Send to the DynStorage trait - I'm still playing around how to not have to worry about the trait objects at other places.

ttiurani commented 3 days ago

Closed as implemented with #139.

datrs / hypercore