Switch to async / std::future::Future across codebase

[zmrow]

tough-ssm creates its own tokio runtime because it needs to use the rusoto which is async. However, this causes a problem if you're trying to use tough-ssm anywhere else that is async... (because multiple runtimes)

We've seen errors like this:

...
thread 'main' panicked at 'Cannot start a runtime from within a runtime. 
This happens because a function (like `block_on`) attempted to block the current thread 
while the thread is being used to drive asynchronous tasks.', 
...

We should probably change the KeySource trait to be async using the async-trait crate

[iliana]

It would probably be more accurate to say "convert tough to async / std::future::Future code" than to identify tough-ssm as what needs to change :)

[iliana]

@webern can I grab this issue as well? The new Datastore trait proposed in #417 should probably also be #[async_trait].

[iliana]

(I guess more specifically: I am rashly assuming this is still something we want to do before tough 1.0; is that correct?)

[webern]

Yes, we should do this.

Now that I understand the Rust async world slightly better, and since reqwest::blocking is very problematic (since it instantiates a tokio runtime), here's what I think we should/shouldn't do:

• We should change over (pretty much) all existing functions and traits to async ASAP. (This will happen naturally because of the red function/blue function nature of async)
• We should not attempt to provide a backward-compatible blocking API. (Based on the reqwest example it seems to do so is fraught and makes foot-guns hard to see)

We should get input from @tjkirch and @zmrow on these points.

[iliana]

Progress update: I have tough building, except for the editor module. trait TargetsWalker's use of a Fn trait for what will become an async function is going to take some refactoring.

tough/tough/src/editor/signed.rs

Lines 592 to 600 in 446615b

	fn walk_targets<F>(
	&self,
	indir: &Path,
	outdir: &Path,
	f: F,
	replace_behavior: PathExists,
	) -> Result<()>
	where
	F: Fn(&Self, &Path, &Path, PathExists, Option<&TargetName>) -> Result<()>,

[webern]

Hi @iliana it sounds like you got pretty close with this. I wonder if there is a way to collaborate on it. By its nature, a partial implementation cannot be merged, so it's tricky.

[iliana]

I'll push my WIP branch later today, I shifted onto using tough as-is (synchronously) for expediency's sake a few weeks back.

[webern]

I shifted onto using tough as-is

We just did this in another place as well, and it made me sad. If you're using a tokio runtime, then I think the safe thing to do is to spawn a thread for tough calls. Is this what you did?

// tokio runtime exists
    thread::spawn(|| {
        tough::blah();
    }).join().expect("I failed")

[iliana]

Yeah I ended up doing something similar with spawn_blocking: https://github.com/oxidecomputer/omicron/blob/e8727457d2132eb1a22b5f50352eb961ee2c147e/nexus/src/nexus.rs#L2273-L2276

[webern]

@iliana I started looking at this tonight. I think it all starts with a decision on the Transport trait's fetch interface. Did you go with something like this?

#[async_trait::async_trait]
pub trait Transport: Debug + DynClone {
    /// Opens a `Read` object for the file specified by `url`.
    async fn fetch(
        &self,
        url: Url,
    ) -> Result<
        Box<dyn futures::Stream<Item = Result<bytes::Bytes, TransportError>> + Send>,
        TransportError,
    >;
}

Or maybe...

#[async_trait::async_trait]
pub trait Transport: Debug + DynClone {
    /// Opens a `Read` object for the file specified by `url`.
    async fn fetch(&self, url: Url) -> Result<Box<dyn futures::AsyncRead + Send>, TransportError>;
}

Changing to a Stream seems... hard? But std::stream::Stream exists in nightly, so I guess it will become part of the std lib sometime, so maybe that's the best choice?

[iliana]

I finally got over whatever mental hurdle I had to push the WIP i have: develop...iliana:async-or-swim

[iliana]

@webern so for Transport I specifically used tokio::io::AsyncRead, but I'm not sure how I feel about that decision? I think it's more appropriate and probably easier to use than Stream<Item = Result<Bytes, _>>... but maybe Stream has advantages I'm not thinking about.

However I have been building without the http feature in my WIP branch so far. I'm not sure yet whether I can convert from a Stream over Bytes into an AsyncRead?

[qm3ster]

@iliana there's tokio_util::io::StreamReader ~

[matze]

Hi, is there any ETA when it would be possible to use tough properly in an async environment?

[webern]

Currently, we do not have a reliable ETA for this request.

[matze]

Is there a way to help here? For example picking up the work from @iliana ? It'd be really nice to a) avoid the multiple executors thing and b) better integration with existing async code.

[stmcginnis]

Thanks @matze - contributions are definitely welcome if you are able to take a look.

[matze]

I think the first question is, if it's okay to lock into and depend on tokio or to be executor-agnostic? tokio is the de-facto standard, on the other hand I can in principle see the need to run tough in a no_std environment with a different executor. I tried rebasing @iliana's branch but of course it does not apply after so much time.

[cbgbt]

I see that @iliana's branch uses tokio for async fs access, which makes a lot of sense. I'm not sure if there are good nostd alternatives for that. (A cursory search doesn't look promising)

Barring any good suggestions, I think tokio is probably the right way forward, though I do agree that there's value in enabling nostd.

[phu-cinemo]

I did a conversion to async based on streams as a replacement for std::io::Read. I'm on the same team as @matze btw, so no work wasted here.

[webern]

Closed by #687!