async: blocking client API

[jebrosen]

Adds a blocking variant of the APIs in rocket::local:: which is more convenient to use in testing. Most tests are currently async only because the API is, and the use of #[rocket::async_test] and .awaits sprinkled throughout is just ceremony.

An alternative approach could be to support only a blocking API and keep the async bits internal-only, but since requests can and will be executed concurrently in a live server I felt uncomfortable imposing this restriction. I believe that the async-capable test harness can be useful for simulating routes under load (e.g. interactions with databases or external APIs) without needing an HTTP client test harness.

Implementation Overview

The blocking Client spawns a single background thread on construction. This thread runs a single-threaded tokio Runtime, and waits for a signal that the associated Client has been dropped. Methods on the blocking Client, LocalRequest, and LocalResponse use the runtime's Handle::block_on method to expose a synchronous API on top of the existing asynchronous Client. This is similar to the blocking API for reqwest, but it takes advantage of the recently added Handle::block_on method in tokio to avoid an additional channel for communication to/from the runtime.

Method signatures and documentation are simply copied from the blocking to the non-blocking version, and method bodies have been replaced with client.block_on() as necessary. Due to the large amount of similarity, diff -u client.rs blocking/client.rs will be useful for reviewing this.

Todo

• Currently, all documentation is copied directly from the existing API. This raises two new issues:
  • Where to highlight the differences, and where to leave documentation identical.
  • How to keep these in sync in the future.
• Add explanations and examples to the guide-level documentation.

[SergioBenitez]

Awesome.

My concerns can be summarized by this diff: https://paste.rs/W1Y. (https://paste.rs/W1Y.diff)

In short:

• Why not store the Runtime directly behind a Mutex? This way, we don't need a special dance to keep the runtime on a block_on.
• I'd rather have the self.inner = foo; self in each method than the _map. I don't care too much. In any case, the _map can be written more cleanly as: self.inner = f(self.inner); self.
• I renamed crate::local as aio just for my own sanity. This isn't a request.

I have ideas on how to clean things up re: documentation/duplication/keeping things in sync, but let's agree on an implementation first.

[jebrosen]

@SergioBenitez were you planning on adding more methods to Client / or already have some locally, or shall I work on adding those?

[SergioBenitez]

@jebrosen Yes. I'm finishing off the Client changes now. What's left after this is to do effectively the same thing for LocalRequest and LocalResponse as has been done with Client re:deduplication.

[SergioBenitez]

@jebrosen Pushed! You'll see that I modified the deduplication macro to something that I believe is much cleaner and easier to work with. It also generates all of the examples, so no documentation is duplicated either, at the small cost of losing a bit of detail. Finally, I added an Rc to blocking::Client to make it !Send; RefCell is Send.

[jebrosen]

You'll see that I modified the deduplication macro to something that I believe is much cleaner and easier to work with. It also generates all of the examples, so no documentation is duplicated either, at the small cost of losing a bit of detail.

Yeah, this does look much easier to work with!

Finally, I added an Rc to blocking::Client to make it !Send

This begs the question: it obviously can't be Sync (we want &self -> &mut Runtime without wasting a Mutex), but is there any harm in it being Send? Are we future-proofing by not promising Send too eagerly? Whatever the case, a comment would be good since the main API of Rc (Clone and Drop) is never actually used.

[SergioBenitez]

@jebrosen The main reason I'd like to be !Send is so that the following is a compile-time error:

#[async_test]
async fn test() {
    let client = local::blocking:Client::new(rocket());
    let res = client.get("/").dispatch();
}

Otherwise, we'll be executing block_on in an async environment, which will invariably result in a panic.

[jebrosen]

Ah. That's not actually sufficient, since the client is not held across an await: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=a0a4bbe10eab10576493ac31a5209cc1

[SergioBenitez]

@jebrosen Of course. Bummer. I suppose a panic isn't too bad, given that these are tests, though it may result in false-positives for #[should_panic] tests.

One idea is to insert a dummy .await at the beginning of the user's function to force the await point. Something like:

#[async_test]
async fn test() {
    let _rocket_inserted_await = dummy().await;
    let client = local::blocking:Client::new(rocket());
    let res = client.get("/").dispatch();
}

Would love any other ideas you might have, or perhaps you feel that that this isn't something to be too concerned about?

[SergioBenitez]

Alright! I believe, barring a few doc fixes, this is complete! I rebased on async after the Manifest -> Cargo changes, which blew away commit history, but I'll restore things when I actually merge.

There are some sweeping changes here. Here's my attempt at iterating them:

• Request is now Clone, but only privately. This is because we need to know that there are no outstanding borrows to internal structures to successfully clone everything. We know this in LocalRequest, so we use it there.
• We use the atomic crate to atomically update Method. We really don't want to not get the latest Method on clone. If we want to eschew the dependency, we can manually use an AtomicUsize and convert between it and Method.
• mut_dispatch is gone. It causes issues and makes reasoning about the soundness of unsafe code more difficult. I've also figured out how to get rid of the one-line of unsafe code remaining, even though I'm convinced it's correct.
• LocalResponse exposes Response as read-only. This makes sense to me; you have the Response already, what is there to modify? The body reader methods were changed to into_string() and into_bytes(). Perhaps we should add an into_reader() or something of the sort? Or into_raw() -> ResponseBody?
• Body exposes a read-only mechanism to determine what the size of the body is, if it is known beforehand. A few tests make use of this.
• Response::body() is now Response::body_mut(). Response::body() -> &Body was added.

@jebrosen Please do feel free to comment on any of this!

[jebrosen]

This all looks really great!

• RE Request and Clone - I expect it could be safe and useful to clone a request that has never been dispatched (and that does not have any request-local state?). I believe the problems here are mostly related to request-local state, but rocket.rs does swap the method around a bit when auto-handling HEAD requests. This point is orthogonal to unsafe, although unsafe can "make things worse" as we saw in Clone implementation for LocalRequest is unsound #1312.
• into_reader or into_raw may be helpful for testing "unending" responses, or responses that can be influenced by making other requests to the server. Such methods may actually be necessary to be able to test request flows such as "start request A, read start of response A" -> "make a second request B" -> "read more of response A" -> "make a third request C" -> "read rest of response A".

I think all that's left to do is docs:

• docs on structs (Client, LocalRequest, LocalResponse)
• docs on modules (local, local::asynchronous, local::blocking)
• guide-level docs
• ensuring all the docs are correct and useful. For example, I see one or two doc examples that are missing .await on asynchronous::Client.

I will start those by copying text from the 0.4 docs, adding in words such as "asynchronous" or "blocking" as necessary.

[SergioBenitez]

@jebrosen Okay, I think I'm happy with this implementation + docs. I think the docs here strike a good balance between deduplication and having the relevant information in the right place.

Would love for you to take a glance at the implementation, @jebrosen, see if I missed anything. Otherwise, I propose that we migrate all of the examples that can use the blocking set of APIs to the blocking variant, after which I propose we merge.

[jebrosen]

This looks great! I reviewed the documentation text and fixed one or two docs links, and I'm working on switching to blocking in examples. I'll also look for and possibly migrate documentation tests outside of local::. Would you prefer for tests in core/lib/tests, contrib/lib/tests, etc. to use the asynchronous or blocking APIs?

[SergioBenitez]

Would you prefer for tests in core/lib/tests, contrib/lib/tests, etc. to use the asynchronous or blocking APIs?

Yeah. Unless I'm missing something, I'd say if we can use the blocking API, let's use it.

[SergioBenitez]

Okay, minus the guide, I'm satisfied with merging this. If you'd like to take a crack at fixing up the guide for this, I'm happy to accept anything. I think we should effectively only discuss one of the two variants, ideally the blocking one, and then say, "You'll have noticed blocking up there. That's because ... Sometimes, you'll need async, such as when [...]. so there's asynchronous! It works the exact same way. See [here] for an example and [here] for more docs."

[jebrosen]

Alright, I've added a note (hopefully formatted correctly). We don't currently have any examples or tests where asynchronous dispatching actually makes a difference, so the example is a bit thin.

[SergioBenitez]

I added an example where async testing is really required.

Funny enough, this revealed that Chrome doesn't concurrently dispatch two requests to the same server/path combo. Instead, it waits for the first to finish before dispatching the second. Thus, testing this example in the browser via two tabs to /barrier in Chrome gives the appearance that the server is blocking. Something to keep in mind for "bug reports" in the future.

[jebrosen]

Funny enough, this revealed that Chrome doesn't concurrently dispatch two requests to the same server/path combo. Instead, it waits for the first to finish before dispatching the second. Thus, testing this example in the browser via two tabs to /barrier in Chrome gives the appearance that the server is blocking. Something to keep in mind for "bug reports" in the future.

Very interesting. I replicated in Chromium, and Firefox sends the requests simultaneously. I expect that this behavior could depend on many factors in either browser, such as HTTP/2 and user configuration.

[SergioBenitez]

Excellent. Working on merging now.

[SergioBenitez]

Merged in 7224970 ... 4419dd3. Woo!