ASU-cubesat / cfdp-rs

A rust implementation of the CCSDS File Delivery Protocol (CFDP)
MIT License
9 stars 2 forks source link

Move to Async Runtime #6

Closed mkolopanis closed 1 year ago

mkolopanis commented 1 year ago

With all the communication channels and File I/O, this project could probably benefit from an async runtime.

xpromache commented 1 year ago

Hey Matthew, are you still working on this issue?

I was experimenting a bit with async and I think I will have a async version of cfdp-rs soon.

We still want to go async direction, right?

mkolopanis commented 1 year ago

I definitely still think it's a direction I'd like to take the crate in. I've been getting a lot more familiar with asnyc in other codes.

If you think you've got something ready to go soon I'd love to see it.

xpromache commented 1 year ago

I definitely still think it's a direction I'd like to take the crate in. I've been getting a lot more familiar with asnyc in other codes.

If you think you've got something ready to go soon I'd love to see it.

I'm struggling a bit to get the series_f1 tests to run properly. When I run them one by one they run fine but when I start them together some fail.

The reason is that the tests share the same daemons (local and remote) but tokio starts a new separate runtime for each test. The daemons however would be associated to the first runtime created and as soon as that test is finished the runtime would be dropped, the daemons would be dropped as well and all the other tests still not finished would fail.

To solve I need to either:

xpromache commented 1 year ago

So after playing more with the tests (I also made a pull request to remove some duplicates) I think option 2 is actually the best. I can create a runtime together with the daemons such that the cfdp-core is async but the tests will remain sync.

The TestUserHalf will keep a reference to the tokio runtime and make the bridge between the sync tests and the async daemon. This way the tests will change very little if at all. Cargo test will also be happy, it can run each test in its own thread but they will all connect to the async daemon which runs in a different thread.

My current async code is here: https://github.com/xpromache/cfdp-rs/tree/async

xpromache commented 1 year ago

So here is what I got: https://github.com/xpromache/cfdp-rs/tree/async-with-sync-tests

I create the runtime and store it together with the filestore in a new once fixture StaticAssets (sorry for the silly name, I couldn't come up with something better, I would be happy to change if you have a better one).

All the tests functions run into their own thread (no need for async and #[tokio::test] accessing that single tokio runtime.

There are some spurious test failures, I will be looking into them later.

mkolopanis commented 1 year ago

Sorry you caught me in the middle of my night. I like the approach you took though, nice to not have all the tests requiring their own runtime. StaticAssets seems like a perfectly reasonable name to me.

I've noticed that MacOs still gives some spurious failures even in the current version. Also noticed when I try to tweak transports and timings I can get spurious failures on my own system. Entirely possible it is bandwidth overloads or something of the sort.

xpromache commented 1 year ago

Ok, can you try again please?

I fixed one bug: when reshuffling the code, the timeout computation ended up outside the receive loop (meaning it was computed once and never updated)

mkolopanis commented 1 year ago

Seems to be working now on my machine well enough.