Remove BoxFuture from core and replace with manual future impl

[faern]

So here is an initial step in reducing boxed futures. Instead of saying that the Transport trait should always return BoxFuture<Vec<u8>, E> we can be more general and say that it must specify a type Future: Future<Item = Vec<u8>> more or less. Now I just moved the BoxFuture into the http crate and named it HttpFuture. So in practice this becomes the exact same thing. But no longer enforced in core, which makes it possible to now write transport implementations that return non-boxed futures.

I don't want to put a Clone bound on the error type, so that forces me to only allow calling poll once on an RpcRequest that is in the error state (can only happen if the serialization of the method call failed).

---

This change is 

[jvff]

I agree with the decision to not require Error: Clone. If the user wants to avoid any panics, he can always .fuse() the future.

I never know if polling twice is considered "undefined behavior" by default, but perhaps it's a good idea to document the panic possibility somewhere.

---

Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

core/src/lib.rs, line 172 at r1 (raw file):

    fn poll(&mut self) -> futures::Poll<Self::Item, Self::Error> {
        match self.transport_future.poll() {

This is mostly a style preference, but I usually do:

let poll_result = self.transport_future.poll().chain_err(ErrorKind::TransportError);
let response_raw = try_ready!(poll_result);

trace!(
    "Deserializing {} byte response to method \"{}\" with id {:?}",
// ...

---

core/src/lib.rs, line 181 at r1 (raw file):

                    self.id
                );
                response::parse::<T>(&response_raw, &self.id).map(|t| Async::Ready(t))

This turbo-fish appears to be unnecessary. Or is this intentional for additional clarity?

---

core/src/macros.rs, line 33 at r1 (raw file):

            $(
                $(#[$doc])*

I suggest calling this attr or something similar to avoid being unnecessarily specific. Same for struct_attr above.

---

core/src/response.rs, line 33 at r1 (raw file):

        Output::Success(success) => {
            trace!("Received json result: {}", success.result);
            serde_json::from_value::<R>(success.result)

This turbo-fish is also unnecessary, like above.

---

core/src/response.rs, line 36 at r1 (raw file):

                .chain_err(|| ErrorKind::ResponseError("Not valid for target type"))
        }
        Output::Failure(failure) => Err(ErrorKind::JsonRpcError(failure.error).into()),

Another small nitpick, bail!(ErrorKind::JsonRpcError(failure.error)) is slightly shorter, so it might be marginally easier to read.

Nothing serious, just a suggestion.

---

http/src/lib.rs, line 123 at r1 (raw file):

/// Future type returned from `HttpTransport`.
pub type HttpFuture<T, E> = Box<Future<Item = T, Error = E> + Send>;

Can the fact that HttpFuture<T, E> == BoxFuture<T, E> cause confusion? However, pub struct HttpFuture<T, E>(Box<Future<Item = T, Error = E> + Send>); might be overkill, especially because of impl<T, E> Future for HttpFuture<T, E> boilerplate.

I would suggest just calling it BoxFuture as well (or import the alias from somewhere else, perhaps defining it in the crate root module).

---

Comments from Reviewable

[faern]

I have seen standard futures with this behavior, without even documenting it: mullvad/system-configuration-rs#8 (comment)

And the docs for the Future trait says "Once a future has finished, clients should not poll it again.". And a future that has returned an error is finished. So we don't break that contract.

---

Review status: 2 of 4 files reviewed at latest revision, 7 unresolved discussions.

---

core/src/lib.rs, line 172 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

This is mostly a style preference, but I usually do:

let poll_result = self.transport_future.poll().chain_err(ErrorKind::TransportError);
let response_raw = try_ready!(poll_result);

trace!(
    "Deserializing {} byte response to method \"{}\" with id {:?}",
// ...

Interesting. I had completely missed the existence of try_ready!. Yeah, I could really simplify this method with that trick. Thanks!

---

core/src/lib.rs, line 181 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

This turbo-fish appears to be unnecessary. Or is this intentional for additional clarity?

It indeed works without it. Either it was required in some previous iteration of the code or I just thought it was and added it. No, not added for clarity on purpose. I removed it.

---

core/src/response.rs, line 33 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

This turbo-fish is also unnecessary, like above.

Removed.

---

core/src/response.rs, line 36 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Another small nitpick, bail!(ErrorKind::JsonRpcError(failure.error)) is slightly shorter, so it might be marginally easier to read.

Nothing serious, just a suggestion.

True. I have mixed feelings about bail!. It's handy at times. But as I explained when I reviewed your PR I like to avoid early returns and trying to code in expressions. bail! is an early return internally... But so is try! and ? and I don't have anything against them so I guess I'm just silly. Changed to bail! now.

What I think I'm against is when a bail! end up in the exact same place where a simple expression could be used, like above. I have less problem with:

...
if check_something {
    bail!(ErrorKind::SomethingWrong);
}
...

Because then it's used in the same way as ? and ensure!.

---

http/src/lib.rs, line 123 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Can the fact that HttpFuture<T, E> == BoxFuture<T, E> cause confusion? However, pub struct HttpFuture<T, E>(Box<Future<Item = T, Error = E> + Send>); might be overkill, especially because of impl<T, E> Future for HttpFuture<T, E> boilerplate.

I would suggest just calling it BoxFuture as well (or import the alias from somewhere else, perhaps defining it in the crate root module).

It is defined in the http crate root module already? I named it as such so it could be swapped for a "real" implementation later without changing the name. As a user of HttpFuture you should not have to care how it's implemented, just that it implements Future. No one is supposed to compare it with BoxFuture because they are not conceptually equal, just happen to be an identical type :)

But I don't have an extremely strong opinion. I like to express that it's a HttpFuture, but if you really prefer BoxFuture I can rename it.

---

Comments from Reviewable

[faern]

Review status: 2 of 4 files reviewed at latest revision, 6 unresolved discussions.

---

core/src/macros.rs, line 33 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

I suggest calling this attr or something similar to avoid being unnecessarily specific. Same for struct_attr above.

You mean because it could also represent some other attribute, such as #[cfg(...)]? Yeah, make sense. It was only inteded to be used for documentation when I wrote it.

---

Comments from Reviewable

[faern]

Btw. Panicing is not undefined behavior. What a panic is and how it's handled is fully defined and known. Undefined behavior is unsafe in Rust, but panics are not.

---

Review status: 1 of 4 files reviewed at latest revision, 6 unresolved discussions.

---

Comments from Reviewable

[faern]

Review status: 1 of 4 files reviewed at latest revision, 7 unresolved discussions.

---

core/src/lib.rs, line 181 at r2 (raw file):

            "Deserializing {} byte response to method \"{}\" with id {:?}",
            response_raw.len(),
            self.method,

What do you think about storing the method string in RpcRequest? It's only used for logging. I now went ahead and removed it since it slimmed down a few things and could avoid one string clone().

The trace logging will still print the expected ID of the response, and since we trace log when sending the request it's still possible to correlate responses with requests if one read the logs. So it did not remove something crucial for debugging, only made it a bit less convenient to follow the flow.

---

Comments from Reviewable

[pronebird]

Reviewed 1 of 4 files at r1, 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions.

---

Comments from Reviewable

[jvff]

"Undefined behavior" might have been a poor choice of words, but it was the closest I got to what I meant. The contract doesn't specify what happens if a Future is polled after it has completed. Not saying "it will panic" or something is what I meant by "undefined behavior", but perhaps unclear or inconsistent behavior might be better terms.

I think not documenting the panic is okay though, if the user breaks the contract, it's better if he finds out ASAP.

---

Reviewed 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

core/src/lib.rs, line 181 at r2 (raw file):

Previously, faern (Linus Färnstrand) wrote…

What do you think about storing the method string in RpcRequest? It's only used for logging. I now went ahead and removed it since it slimmed down a few things and could avoid one string clone().

The trace logging will still print the expected ID of the response, and since we trace log when sending the request it's still possible to correlate responses with requests if one read the logs. So it did not remove something crucial for debugging, only made it a bit less convenient to follow the flow.

If it's only used for logging and avoids a clone, I think it's a good change. If we need to pass it into InnerRpcRequest some time in the future we can also pass a reference to it as a parameter to the necessary method(s).

---

core/src/response.rs, line 36 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

True. I have mixed feelings about bail!. It's handy at times. But as I explained when I reviewed your PR I like to avoid early returns and trying to code in expressions. bail! is an early return internally... But so is try! and ? and I don't have anything against them so I guess I'm just silly. Changed to bail! now.

What I think I'm against is when a bail! end up in the exact same place where a simple expression could be used, like above. I have less problem with:

...
if check_something {
    bail!(ErrorKind::SomethingWrong);
}
...

Because then it's used in the same way as ? and ensure!.

I see your point. I usually avoid the extra key presses, but I don't mind it being different and I believe consistency is more important. If you prefer to prioritize expression-returns instead of bail! or explicit returns in the end of functions, I'll adopt that style as well.

---

http/src/lib.rs, line 123 at r1 (raw file):

Previously, faern (Linus Färnstrand) wrote…

It is defined in the http crate root module already? I named it as such so it could be swapped for a "real" implementation later without changing the name. As a user of HttpFuture you should not have to care how it's implemented, just that it implements Future. No one is supposed to compare it with BoxFuture because they are not conceptually equal, just happen to be an identical type :)

But I don't have an extremely strong opinion. I like to express that it's a HttpFuture, but if you really prefer BoxFuture I can rename it.

Now I understand why. I think it's a good stepping stone for a proper type in the future. Perhaps adding a // TODO: Create a stand-alone type might notify other readers why it exists.

---

Comments from Reviewable

[faern]

Review status: 3 of 4 files reviewed at latest revision, 3 unresolved discussions.

---

http/src/lib.rs, line 123 at r1 (raw file):

Previously, jvff (Janito Vaqueiro Ferreira Filho) wrote…

Now I understand why. I think it's a good stepping stone for a proper type in the future. Perhaps adding a // TODO: Create a stand-alone type might notify other readers why it exists.

I ended up removing the HttpFuture alias instead. It was only referenced once, all other places used Self::Future anyway so it did not save us from any typing anyway.

---

Comments from Reviewable

[jvff]

Reviewed 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable