{"action":"opened","issue":{"url":"https://api.github.com/repos/timberio/vector/issues/1091","repository_url":"https://api.github.com/repos/timberio/vector","labels_url":"https://api.github.com/repos/timberio/vector/issues/1091/labels{/name}","comments_url":"https://api.github.com/repos/timberio/vector/issues/1091/comments","events_url":"https://api.github.com/repos/timberio/vector/issues/1091/events","html_url":"https://github.com/timberio/vector/issues/1091","id":512559857,"node_id":"MDU6SXNzdWU1MTI1NTk4NTc=","number":1091,"title":"Implement a proper topology stop  strategy","user":{"login":"LucioFranco","id":5758045,"node_id":"MDQ6VXNlcjU3NTgwNDU=","avatar_url":"https://avatars0.githubusercontent.com/u/5758045?v=4","gravatar_id":"","url":"https://api.github.com/users/LucioFranco","html_url":"https://github.com/LucioFranco","followers_url":"https://api.github.com/users/LucioFranco/followers","following_url":"https://api.github.com/users/LucioFranco/following{/other_user}","gists_url":"https://api.github.com/users/LucioFranco/gists{/gist_id}","starred_url":"https://api.github.com/users/LucioFranco/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/LucioFranco/subscriptions","organizations_url":"https://api.github.com/users/LucioFranco/orgs","repos_url":"https://api.github.com/users/LucioFranco/repos","events_url":"https://api.github.com/users/LucioFranco/events{/privacy}","received_events_url":"https://api.github.com/users/LucioFranco/received_events","type":"User","site_admin":false},"labels":[{"id":1282237504,"node_id":"MDU6TGFiZWwxMjgyMjM3NTA0","url":"https://api.github.com/repos/timberio/vector/labels/domain:%20topology","name":"domain: topology","color":"212f3e","default":false},{"id":1448711477,"node_id":"MDU6TGFiZWwxNDQ4NzExNDc3","url":"https://api.github.com/repos/timberio/vector/labels/needs:%20approval","name":"needs: approval","color":"feca57","default":false},{"id":1257936363,"node_id":"MDU6TGFiZWwxMjU3OTM2MzYz","url":"https://api.github.com/repos/timberio/vector/labels/type:%20bug","name":"type: bug","color":"ee5353","default":false},{"id":1282087182,"node_id":"MDU6TGFiZWwxMjgyMDg3MTgy","url":"https://api.github.com/repos/timberio/vector/labels/type:%20enhancement","name":"type: enhancement","color":"5f27cd","default":false},{"id":1293808498,"node_id":"MDU6TGFiZWwxMjkzODA4NDk4","url":"https://api.github.com/repos/timberio/vector/labels/type:%20tech%20debt","name":"type: tech debt","color":"5f27cd","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":0,"created_at":"2019-10-25T14:48:53Z","updated_at":"2019-10-25T14:48:53Z","closed_at":null,"author_association":"MEMBER","body":"# Motivation\r\n\r\nCurrently, our topology stop method attempts to shut down the topology tasks by selecting against them and dropping any resources. This is incorrect, we should instead notify the task that we need to shut down and let it gracefully shutdown.\r\n\r\nPrior art:\r\n\r\nhttps://docs.rs/tokio-evacuate/\r\nhttps://github.com/linkerd/linkerd2-proxy/blob/master/linkerd/app/core/src/serve.rs#L13\r\n\r\n# Proposal\r\n\r\nOur codebase currently does this https://github.com/timberio/vector/blob/master/src/topology/builder.rs#L128 which will force cancel a task. In the context of TCP this is fine because we create a second [tripwire](https://github.com/timberio/vector/blob/master/src/sources/util/tcp.rs#L75) that when the original task gets drop will fire and thus cancel all of its child tasks. \r\n\r\nThis though does not work for something like hyper that wants a signal to shutdown as shown here https://docs.rs/hyper/0.13.0-alpha.4/hyper/server/struct.Server.html#method.with_graceful_shutdown. What this means is that we were never originally \"gracefully\" shutting down but forcing the task to quit.\r\n\r\nMy proposal is to invert this by providing each spawned task a handle that can produce a future that will be notified once the shutdown process has started. Each handle is correlated with one task. That task should be considered part of the group of tasks connected to that one instance of the topology. When you reload the topology we should shutdown all tasks that were spawned from that config. The shutdown should first signal each task so they can start their clean up. Once they have all finished their clean up the handle gets dropped. Once, all handles are dropped the main shutdown future can complete. This main future that is called from `Topology::stop` can be selected against with a timer to produce a graceful shutdown period then a force shutdown if tasks hang.\r\n\r\nHere is an example I threw together last night that shows how we might be able to expose this concept to users that want to implement sources, transforms, and sinks. https://github.com/LucioFranco/iquit/blob/master/src/work.rs#L24\r\n\r\nOne big reason, I think we should provide a better api as well as properly implementing this is because getting shutdown right is hard. Because its hard, it will be difficult to expect contributors to properly implement it manually like how the TCP source handles it. With a nice api to use and good documentation, we can ensure that all our sources, transforms, and sinks properly shutdown in the future.\r\n\r\nIdeally, we could provide something like this:\r\n\r\n```rust\r\nlet shutdown = Shutdown::new();\r\n\r\n// This will use _some_ executor to spawn a task.\r\n//\r\n// This internally will register a new task, get a handle to that\r\n// registration. Then call this closure with the handle so that the\r\n// inner task future can do something with it. This will also wrap\r\n// the returned Future from `SourceConfig::build` with a select against a\r\n// oneshot channel like we do now so that we can force shut it down\r\n// if it does not gracefully shutdown.\r\nshutdown.spawn(|handle| source.build(name, globals, out, handle));\r\n\r\n// Now lets attempt to shut it down\r\nif let Err(_) =\r\nrt.block_on(shutdown.shutdown_with_timeout(Duration::from_secs(60))) {\r\n\t// Graceful shutdown failed so lets force shutdown everything\r\n\trt.block_on(shutdown.force());\r\n}\r\n```\r\n\r\ncc @lukesteensen @a-rodin "}}
{"actor":{"display_login":"LucioFranco"}}