search

rs-ipfs / rust-ipfs

The InterPlanetary File System (IPFS), implemented in Rust.
Apache License 2.0
1.26k stars 164 forks source link

Use released dependencies #75

Closed dvc94ch closed 4 years ago

dvc94ch commented 4 years ago
aphelionz commented 4 years ago

With regards to domain, it looks like in the graph in #63 that it uses tokio under the hood

dvc94ch commented 4 years ago

We should probably stick to async-std, as it's what libp2p uses and try to find an async-std replacement for it.

aphelionz commented 4 years ago

Seems so! Will need @koivunej to weigh in as the http subcrate has tokio in it at least once in its graph. Maybe we can start by keeping the core crate as close to async-std only as possible and work through the subcrates

koivunej commented 4 years ago

We should probably stick to async-std, as it's what libp2p uses and try to find an async-std replacement for it.

As rust-libp2p supports tokio as well, I don't see them using one. During the discussions we have so far I'm yet to see any other reason to use async-std, but offered a technical reason not to use it (the possibly upcoming scheduler). I would wish more technical reasons would be brought up instead of "X uses it" and I'd happy to learn from such.

While the split in the ecosystem is far from ideal, I think that crates around tokio are more mature for http for example. For DNS side of things in addition to domain there is trust-dns, while of course any resolution mechanism could be used via any threadpooling mechanism (as I understand async-std's task::spawn_blocking is at the moment).

Overall more of a subjective feeling I like the explicitness of tokio runtime starting as compared to implicit, which I feel is one of the reasons I like the rust language in general (being able to reason about code locally). As already mentioned in the chat, I think the ability to force using just a single threaded executor does help with implementing and debugging (I do not know if this can be forced with async-std) as the task starvation is more apparent.

[ ] ctr (by me)

The PR is still pending and I do not know how to make progress on that, other than querying progress every once in a while, I'll try to remember to do that. However if the difference was hit during testing, this would be a difficult one to debug, especially once we get interop tests running on CI.

Is there an overall pressing need to use released dependencies?

dvc94ch commented 4 years ago

the possibly upcoming scheduler

I'm not sure why that's a reason not to use async-std. Is there something wrong with that scheduler?

when it comes to tokio vs async-std, I don't really care much. Would you like to open a PR for moving to tokio 2.0? ~It's probably not that involved~ It should be straight forward.

Is there an overall pressing need to use released dependencies?

Not pressing, but this is a requirement for a code base to be labeled "production ready", also it prevents us from creating releases for rust-ipfs.

aphelionz commented 4 years ago

We should absolutely have the dependencies updated to release versions by the end of the grant cycle (i.e. when we deliver Phase 1.2)

vmx commented 4 years ago

I would wish more technical reasons would be brought up instead of "X uses it" and I'd happy to learn from such.

I sadly don't have those as I haven't really used either of them. Though I'd like to mention that Rust folks at PL are using async-std and so far like it. Please don't read this as "PL uses X, hence you have to use X", read this as "there is knowledge built up in related ecosystems, so it might make sense for this project as well".

Also Polkadot (which is a heavy user of libp2p) is using async-std, hence I'd expect the async-std version of libp2p being tested more heavily. The main author of libp2p also leans towards async-std (see https://github.com/libp2p/rust-libp2p/issues/1318#issuecomment-564591490 also for some discussions about technical points).

If it would be my decision, I'd go for async-std.

koivunej commented 4 years ago

Ok, lets go with async-std then. I assume using warp and transitively tokio-0.2 in rust-ipfs-http is still ok.

koivunej commented 4 years ago

[ ] ctr (by me)

The PR is https://github.com/RustCrypto/stream-ciphers/pull/75 and it is currently stuck on MSRV, or not. Pinged just now.

aphelionz commented 4 years ago

Are we able to move ahead with domain given the async-std decision?

koivunej commented 4 years ago

Domain has been pinned. Not closing this issue as the issue still remains. Of course releasing first version would be great move.

I updated #83 regarding dns issues we'd need to move forward. Though, I am not sure why hasn't removal of the dns code been considered? If we just removed all of the defunct code. It doesn't work nor does it have tests. Removal of the FFU code would at least make things easier -- I am thinking of writing an issue of moving IpfsPath from ipfs-http over to ipfs so that at least would become easier.

Also related is #175 which would allow dropping the aes-ctr patching from here maybe.