{"action":"created","issue":{"url":"https://api.github.com/repos/tokio-rs/tokio-service/issues/11","repository_url":"https://api.github.com/repos/tokio-rs/tokio-service","labels_url":"https://api.github.com/repos/tokio-rs/tokio-service/issues/11/labels{/name}","comments_url":"https://api.github.com/repos/tokio-rs/tokio-service/issues/11/comments","events_url":"https://api.github.com/repos/tokio-rs/tokio-service/issues/11/events","html_url":"https://github.com/tokio-rs/tokio-service/pull/11","id":196260328,"number":11,"title":"Proposal: Back pressure handling + take &mut self","user":{"login":"carllerche","id":6180,"avatar_url":"https://avatars.githubusercontent.com/u/6180?v=3","gravatar_id":"","url":"https://api.github.com/users/carllerche","html_url":"https://github.com/carllerche","followers_url":"https://api.github.com/users/carllerche/followers","following_url":"https://api.github.com/users/carllerche/following{/other_user}","gists_url":"https://api.github.com/users/carllerche/gists{/gist_id}","starred_url":"https://api.github.com/users/carllerche/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/carllerche/subscriptions","organizations_url":"https://api.github.com/users/carllerche/orgs","repos_url":"https://api.github.com/users/carllerche/repos","events_url":"https://api.github.com/users/carllerche/events{/privacy}","received_events_url":"https://api.github.com/users/carllerche/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2016-12-18T06:17:38Z","updated_at":"2016-12-19T23:20:06Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/tokio-rs/tokio-service/pulls/11","html_url":"https://github.com/tokio-rs/tokio-service/pull/11","diff_url":"https://github.com/tokio-rs/tokio-service/pull/11.diff","patch_url":"https://github.com/tokio-rs/tokio-service/pull/11.patch"},"body":"The PR represents my proposal for changes to `Service`.\r\n\r\n`Service::call` is updated to follow `Sink` for handling back pressure. I also changed `Service::call` to take `&mut self` vs. `&self`.\r\n\r\n/cc @aturon @alexcrichton @seanmonstar @danburkert"},"comment":{"url":"https://api.github.com/repos/tokio-rs/tokio-service/issues/comments/268107476","html_url":"https://github.com/tokio-rs/tokio-service/pull/11#issuecomment-268107476","issue_url":"https://api.github.com/repos/tokio-rs/tokio-service/issues/11","id":268107476,"user":{"login":"tailhook","id":321799,"avatar_url":"https://avatars.githubusercontent.com/u/321799?v=3","gravatar_id":"","url":"https://api.github.com/users/tailhook","html_url":"https://github.com/tailhook","followers_url":"https://api.github.com/users/tailhook/followers","following_url":"https://api.github.com/users/tailhook/following{/other_user}","gists_url":"https://api.github.com/users/tailhook/gists{/gist_id}","starred_url":"https://api.github.com/users/tailhook/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/tailhook/subscriptions","organizations_url":"https://api.github.com/users/tailhook/orgs","repos_url":"https://api.github.com/users/tailhook/repos","events_url":"https://api.github.com/users/tailhook/events{/privacy}","received_events_url":"https://api.github.com/users/tailhook/received_events","type":"User","site_admin":false},"created_at":"2016-12-19T23:20:06Z","updated_at":"2016-12-19T23:20:06Z","body":"> Can you clarify what you mean by a \"polling interface\"?\r\n\r\nI mean something similar to `poll()` or `poll_complete()`\r\n\r\n> Note that the PR makes Service resemble Sink somewhat more, but does not include poll_complete, because completion takes place through the returned future.\r\n\r\nYes. But there are usually two strategies of implementing something like a service:\r\n\r\n1. Implement it on a single (say TCP) connection. Then it's hard to have a good strategy to keep a progress on a connection by reacting on polling futures. Because futures might be polled out of order the responses are actually arriving. I mean basically this means you need to parse protocol in any future. Which probably means that you will duplicate work on poll of every future that are in-flight for this connection.\r\n\r\n2. Implement it for a connection or a pool that is spawned separately from the service itself. This basically means that future returned by call is a `Receiver<Response>`. And you send requests to a channel. And it isn't clear what calls `poll_complete` on a channel. Should every future do that? It's probably wasteful. Should none of them? While it will work for channel it's a violation of the Sink interface (which channel implements).\r\n\r\nAm I missing something? Are there good strategies to implement service in this form? Surely, all above makes sense for client service. For server side, this patch doesn't make sense, as you seem to confirm with:\r\n\r\n> Backpressure at the connection-accepting level should go through NewService rather than Service\r\n\r\nAdditionally, I disagree with the following:\r\n\r\n> As @carllerche says, it seems plausible to have a separate polling method here, but also prudent to do this first with inherent methods, and only add it to the core trait once we're confident.\r\n\r\nThe more I think of it the more I think adding it later isn't good idea. The problem is that when you have a service that already works, and then adding `poll()` (i.e. as a default method or as `+ Future`) just breaks it silently. I mean service expects it to be `poll()`ed and user don't do it because it's not needed for service.\r\n\r\n> The most immediate goal is to remove the need for unbounded queues in the client implementation, by instead propagating queue fullness as Service backpressure.\r\n\r\nThe intent is clear. I think sink solves it just fine. And I don't buy the argument of *symmetricity* from #8. For example, the following:\r\n\r\n> however there is nothing preventing you from having Service + Future that includes the poll fn\r\n\r\n.. clearly contradicts that symmetric feature.\r\n\r\nSorry, if I'm starting to be annoying. What I'm trying to say is that this patch doesn't solve an issue nearly as good as Sink solves. And continuing to patch interface that doesn't work with small steps is not much better than to deprecate it until we find a good way to do.\r\n\r\nThis is more of a topic of #8 rather than this issue, so I'm trying to be short on this. But it looks like user-friendly interface is going to be something more high level than `call()` anyway ( see [redis example](https://github.com/tokio-rs/tokio-redis/blob/master/src/lib.rs#L82), same thing in [our http client](https://github.com/popravich/minihttp/blob/master/src/client/client.rs#L172) ). So I believe it's better to do that on an extension trait (because same trait might be used both on an individual connection and on any kind of connection pool). And it isn't a big deal to make an extension trait on service or sink. And if sink solves problem better, then I'm going to use it at the expense of few lines of code, like creating a oneshot future."}}
{"actor":{"display_login":"tailhook"}}