Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NewService is generic over error #5

Merged
merged 4 commits into from
Sep 21, 2017
Merged

Conversation

carllerche
Copy link
Member

Previously, NewService required returning a future yielding io::Error as
the error. This is not needed and is restrictive.

Previously, NewService required returning a future yielding io::Error as
the error. This is not needed and is restrictive.
@withoutboats
Copy link
Contributor

withoutboats commented Sep 15, 2017

What are the use cases for which io::Error is insufficient here?

My concern is basically that if you want to fold multiple NewServices together, you might end up needing to bound their Error types in a way which can be pretty ugly:

fn two_service<A, B, E>(factory1: A, factory2: B) -> Join<A::Future, B::Future> where
     A: NewService,
     B: NewService,
     A::Future: Future<Error = E>,
     B::Future: Future<Error = E>,
{
     factory1.new_service().join(factory2.new_service())
}

A way to mitigate this would be to add yet another associated type to NewService, InitError or something, so that at least you don't have to pass through the Future type to constrain the error type (I assume the same reason that Service's assoc types are on NewService).

Not advocating any particular path, just see three options here (master, this branch, adding another assoc type) and they all have drawbacks.

@carllerche
Copy link
Member Author

carllerche commented Sep 15, 2017

Well, one example would be a TLS handshake.

I would say, in general, Service is pretty decoupled from I/O.

@carllerche
Copy link
Member Author

Also, in the back pressure example, NewService spawns a task... which is unrelated to I/O (https://github.com/carllerche/tower/blob/7f59c78166edaa178a4e687d380a3befab2c1347/examples/channel_service.rs#L103).

@withoutboats
Copy link
Contributor

Both great examples! I'm convinced.

How do you feel about the difference between this PR and adding another associated type? On the one hand, Yet Another Associated Type, but I'm worried people are going to end up having to write <T::Future as Future>::Error somewhat often.

@carllerche
Copy link
Member Author

carllerche commented Sep 18, 2017

@withoutboats would T::Future::Error not work? I thought that was doable if there wasn't any ambiguity? (I am probably wrong).

I would say that T::Future as Future>::Error should be avoided if at all possible.

@withoutboats
Copy link
Contributor

Right now, assoc types of assoc types are always treated as ambiguous, e.g. even this:

trait Foo {
    type Bar: Bar;
}

trait Bar {
    type Baz;
}

fn foo<T: Foo>(_: T::Bar::Baz) { }

I think this could be improved but right now that's the state of things unfortunately.

@carllerche
Copy link
Member Author

@withoutboats do you have any ideas for how to name the associated type?

@withoutboats
Copy link
Contributor

@carllerche:

  • InitError?
  • Name the other error ServiceError?

@carllerche
Copy link
Member Author

@withoutboats I whichever is most common to address should be Error. I'm not sure if that will be the init error or the service error...

InitError could be a good option if we determine the service's error should claim Error.

@carllerche
Copy link
Member Author

I added InitError as an associated type.

It requires this in the impl now, which is a bit unfortunate:

https://github.com/carllerche/tower/blob/65b8eea55a39b7934d5b73ab6dc424e4c985dde4/src/lib.rs#L219

@withoutboats
Copy link
Contributor

withoutboats commented Sep 21, 2017

S has an InitError now too, so you could instead write:

type InitError = S::InitError;

For the function impl, you could instead add an E parameter to the impl and bound R: IntoFuture<Item = S, Error = E>.

@carllerche carllerche merged commit 3a0212d into master Sep 21, 2017
@carllerche carllerche deleted the generic-new-service-error branch September 22, 2017 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants