Simplify client doctest with default Runtime::block_on

[dekellum]

The client module level doctest benefits from succinctness more than examples/client.rs and the guide. Here I change it to use Runtime::block_on() with the following advantages:

• Demonstrates blocking wait for a completed Result<Chunk,Error> which should remain a common pattern outside of fully-asynchronous servers.

• Gives a path to reuse the Client, which can now outlive job(s) without hanging

• Avoids rt::lazy, from_utf8 and some other unnecessary noise

Below is the rustdoc, example section output without all of the source comment framing:

extern crate hyper;
extern crate tokio;

use std::io::{stdout, Write};
use hyper::Client;
use hyper::rt::{Future, Stream};
use tokio::runtime::Runtime;

fn main() {
    let mut rt = Runtime::new().unwrap();
    let client = Client::new();
    let job = client
        // Make a GET /ip to 'http://httpbin.org'
        .get("http://httpbin.org/ip".parse().unwrap())

        // And then, if the request gets a response...
        .and_then(|res| {
            println!("status: {}", res.status());

            // Concatenate the body stream into a single buffer,
            // returning a new future.
            res.into_body().concat2()
        });

    // Run the job, blocking until it is complete
    let body = rt.block_on(job).unwrap();
    // Write body to stdout
    stdout().write_all(&body).unwrap();
}

Note tokio::runtime::Runtime isn't currently re-exported in hyper::rt. I could add it there instead, if desired, though I wonder about the sustainability of this re-export approach? The tokio::runtime::Runtime::block_on method was released in tokio 0.1.7, so before merging or releasing this, (I or someone) should increase the patch release of tokio in Cargo.toml. Alternatively, tokio::runtime::current_thread is available in tokio 0.1.6, but my impression is that this runtime is less general purpose than the default runtime.

I have some related extensions/changes that I'd like to propose with examples/client and therefore the guide as well, but this doctest is simpler and therefore a good starting point to hopefully get some feedback.

[seanmonstar]

Thanks for the detailed PR, I really appreciated it!

I'm not sure about (but could be convinced otherwise!) having the docs suggesting that you should reach for block_on. In general, you should most often be writing asynchronous code to be async, and to need to spawn into an executor, and so by showing that you can use block_on sometimes, I fear it promotes the wrong idea.

Especially since you can easily think that you could use block_on anywhere, but if you're inside another future already on the executor, you actually can't! It will panic, of course, but we've already taught a user the wrong pattern, and they may then have difficulty fixing it.

[dekellum]

Thank you for giving feedback.

Firstly, the use of run as used in the current doctest, will also panic if used in an executor. block_on is just more appropriate for a (doc)test than run, given the potential for a halt or live-lock if the client is not dropped, lazy not used, etc. I think many users will start using hyper 0.12, as a client in particular, from the context of either a simple main() or unit tests, where block_on remains appropriate. But I think your point of warning is still valid. Could a block_on example usage be acceptable if it included some more explanation or warning like the following?

//! *Note*: Using `block_on` (or `run`) will panic if used in an executor,
//! since these methods block.  For fully asynchronous client usage, use 
//! `spawn` instead.  This `block_on` pattern can be applicable as a starting
//! point, in test code, CLI's, or batch processors.
//! See the guide (link) for a fully asynchronous example using `spawn`.

I have also made PR #1568 which uses spawn (as you suggest) in examples/client. Part of the rationale of using block_on in the doctest, is that its usefully different from the examples/client. Another useful note in the doctest is to reference the example and/or the associated guide-level walk-through.

[seanmonstar]

I appreciate several of the word clarifications you've made here. However, I actually think the docs should recommend that a user use run or spawn, and not block_on.

As I mentioned in #1568, if the concern is about forgetting to put it in a lazy future, then perhaps it'd be enough to just include a line comment about that above the lazy(|| { line.

[dekellum]

I've attempting a better problem statement in #1571. If replacing run in unacceptable, these proposals could be reformulated to just provide the alternatives along side.

[hashmap]

I found this PR very helpful, I experienced all pain described in #1571

[seanmonstar]

Thanks for the efforts to improve the documentation, I greatly appreciate it! I'm going to close this for now, and allow discussion to happen in #1571. If we find ways in that issue to improve things, new PRs can always be submitted.