Require poll_ready to be called before call

[carllerche]

This updates the Service contract requiring poll_ready to be called
before call. This allows Service::call to panic in the event the
user of the service omits poll_ready or does not wait until Ready is
observed.

[LucioFranco]

I'm 👍 for this change, it does seem to simplify a lot of the impls.

[jonhoo]

I think

tower/tower-buffer/src/lib.rs

Lines 192 to 200 in 25eb521

	// TODO:
	// ideally we'd poll_ready again here so we don't allocate the oneshot
	// if the try_send is about to fail, but sadly we can't call poll_ready
	// outside of task context.
	let (tx, rx) = oneshot::channel();
	match self.tx.try_send(Message { request, tx }) {
	Err(e) => {
	if e.is_closed() {

can then also be fixed up a little?

[seanmonstar]

I'm not so convinced that panicking so easily is the right call.

What's the motivation, to remove some error variants? There is still code in place to branch to panic, so the branches aren't less there. If we change to #156, then the user doesn't need to see the NotReady variant.

[carllerche]

@seanmonstar

The motivation is to have more defined contract for the Service trait. Prior to this change, Service was defined to say "you sometimes need to call poll_ready before call... but sometimes you don't.. check the implementation's documentation for the specifics of how to use it". This is not great.

With this change, the contract is clearer: you must always get Ready from poll_ready before calling call.

panic vs. Err is more philisophical. IMO, bugs in code that result in violation of API contracts should result in a panic. The code path in question will never succeed and could result in corruption of the value. In which case, the service would need to maintain internal state saying "yo, i'm busted".

Also, I much prefer panics when there is a bug because you get a backtrace! If call returns Err (future that errors), that future will bubble way back up the call stack and best case you will get a log entry in your server saying "this request failed". A lot of the context will be lost.

Ideally, the panic is isolated to the task that is handling the request and does not impact unrelated tasks in the process.

I prefer to use reserve Result for error scenarios that happen despite correct code due to a runtime situation.

Also, while the call function still needs a branch for the panic (though it can probably be implemented as an assert! which would be clearer), the ResponseFuture struct is smaller and its state machine is simpler.

nope

[seanmonstar]

I agree with "panic on programmer error", my reservation was that I thought the buffer instance could panic even if the programmer didn't do anything wrong. 👍