{"action":"created","issue":{"url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/1297","repository_url":"https://api.github.com/repos/measurement-kit/measurement-kit","labels_url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/1297/labels{/name}","comments_url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/1297/comments","events_url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/1297/events","html_url":"https://github.com/measurement-kit/measurement-kit/pull/1297","id":250034437,"number":1297,"title":"Better life-cycle model for long-running tasks","user":{"login":"bassosimone","id":337298,"avatar_url":"https://avatars0.githubusercontent.com/u/337298?v=4","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false},"labels":[{"id":103088501,"url":"https://api.github.com/repos/measurement-kit/measurement-kit/labels/enhancement","name":"enhancement","color":"84b6eb","default":true},{"id":103088504,"url":"https://api.github.com/repos/measurement-kit/measurement-kit/labels/question","name":"question","color":"cc317c","default":true},{"id":321612524,"url":"https://api.github.com/repos/measurement-kit/measurement-kit/labels/ready%20for%20review","name":"ready for review","color":"eb6420","default":false},{"id":473102883,"url":"https://api.github.com/repos/measurement-kit/measurement-kit/labels/refactoring","name":"refactoring","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":{"login":"bassosimone","id":337298,"avatar_url":"https://avatars0.githubusercontent.com/u/337298?v=4","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false},"assignees":[{"login":"bassosimone","id":337298,"avatar_url":"https://avatars0.githubusercontent.com/u/337298?v=4","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false}],"milestone":{"url":"https://api.github.com/repos/measurement-kit/measurement-kit/milestones/15","html_url":"https://github.com/measurement-kit/measurement-kit/milestone/15","labels_url":"https://api.github.com/repos/measurement-kit/measurement-kit/milestones/15/labels","id":2704737,"number":15,"title":"v0.8.0","description":"","creator":{"login":"bassosimone","id":337298,"avatar_url":"https://avatars0.githubusercontent.com/u/337298?v=4","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false},"open_issues":11,"closed_issues":0,"state":"open","created_at":"2017-08-16T12:27:48Z","updated_at":"2017-08-16T12:31:51Z","due_on":"2017-09-15T07:00:00Z","closed_at":null},"comments":5,"created_at":"2017-08-14T13:41:59Z","updated_at":"2017-08-16T23:16:39Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/measurement-kit/measurement-kit/pulls/1297","html_url":"https://github.com/measurement-kit/measurement-kit/pull/1297","diff_url":"https://github.com/measurement-kit/measurement-kit/pull/1297.diff","patch_url":"https://github.com/measurement-kit/measurement-kit/pull/1297.patch"},"body":"Motivation\r\n==========\r\n\r\nI recently had a discussion with @hellais where we agreed that writing code for measurement-kit is difficult.\r\n\r\nWhat makes it difficult, are the rules to guarantee that we are deadling with object's life cycle properly.\r\n\r\nCurrent rules\r\n=============\r\n\r\nWe basically have two cases: the case where state is not needed and the case where state is needed. But there is also code that is not following the current rules.\r\n\r\nNo state needed\r\n---------------\r\n\r\nThe former case is the one where you have a one-shot slow operation to which you pass arguments and a callback.\r\n\r\nConsider for example DNS:\r\n\r\n```C++\r\nmk::dns::query(\"IN\", \"A\", \"www.google.com\",\r\n               [](mk::Error error, mk::dns::Message message) {\r\n               // HANDLE ERROR AND PROCESS MESSAGE\r\n           });\r\n```\r\n\r\nIn this _simple_ case you really don't want and need any more state. This code will create state when you initiate the DNS query and destroy it after the callback.\r\n\r\nState needed\r\n------------\r\n\r\nThere are cases where you cannot get away with a simple function taking arguments and a callback. Those are the cases where you need an object that will emit events throghout its lifecycle. An example of that could be the transport:\r\n\r\n```C++\r\nusing namespace mk;\r\nusing namespace mk::net;\r\n\r\nvoid some_func(Var<Transport> txp, Var<Buffer> buff, ...) {\r\n    read(txp, buff, [](Error error) {\r\n        // HANDLE ERROR OR READ DATA\r\n    });\r\n}\r\n```\r\n\r\nIn these cases the approach has been to allocate state using the `Var<>` shared pointer and then to have function operations to which you can pass such state. While operating on state, these operations will also keep it alive, by virtue of `Var<>` being a shared pointer. While this seems neat, it is a bit obscure in that you don't actually _see_ the moment where the specific bit of code will be destroyed. And having it die when the last callback uses it _may not_ be optimal, so that you need to use the `call_soon()` trick described below.\r\n\r\nCode not following rules\r\n------------------------\r\n\r\nAn example of code not following rules is `nettests::Runnable` where we do pass `this` to some lambdas.\r\n\r\nThis is dangerous because it does not guarantee that the object will still be alive when the callback is called, this possibly leading to _use after free_ errors.\r\n\r\nHowever, for `nettests::Runnable` overall the code is safe because we _know_ that the `Runnable` is managed by a `Runner` that receives the `Runnable` as a shared pointer and properly pass such shared pointer around to some lambdas such that it stays alive until it reaches its final state and calls its final callback.\r\n\r\nWhen the final-callback state is reached, the code is careful not to lose the reference to the shared pointer immediately, which may trigger destruction with a stack shaped like this:\r\n\r\n```\r\n    +---------------+\r\n    | test::final() |\r\n    +---------------+\r\n    |   test::a()   |\r\n    +---------------+\r\n    | libevent code |\r\n    +---------------+\r\n```\r\n\r\nAssuming we're in `test::final()`, calling the final callback and losing the shared pointer reference is dangerous in that this _might_ cause destruction and, hence, when we return to `test::a()`, we can use the object after it has been freed.\r\n\r\nThe solution of this problem is to use `Reactor::call_soon()` such that, when we call the final callback, the stack is instead shaped like this:\r\n\r\n```\r\n    +---------------+\r\n    | test::final() |\r\n    +---------------+\r\n    | libevent code |\r\n    +---------------+\r\n\r\n```\r\n\r\nSo, we just call the final callback, trigger destruction, but since we have only libevent code below on the stack, this means we cannot have a use-after-free condition.\r\n\r\nProposal\r\n========\r\n\r\nThere is one use case where writing code for measurement-kit is actually simpler and less rules are involved.\r\n\r\nThat is the case where you write a simple main, allocate on the stack all the objects you need, start the reactor and, in its \"initial event\" callback, start all the async operations (be them I/O, threads, or delayed calls) you need. This includes setting a final state handler, which calls `reactor->stop()`. E.g.:\r\n\r\n```C++\r\nusing namespace mk;\r\nVar<Reactor> reactor = Reactor::make();\r\n// Note: nonexistent API here only to make a point\r\ndns::Client client{reactor};\r\nclient.set_dns_server(\"8.8.8.8\", 53);\r\nreactor->run_with_initial_event([&]() {\r\n    client.query(\"IN\", \"A\", \"www.google.com\",\r\n                 [](Error error, dns::Message message) {\r\n                     // DO WHAT YOU NEED\r\n                     reactor->stop();\r\n                 });\r\n});\r\n```\r\n\r\nThis is very neat for the following reasons:\r\n\r\n- all you need is allocated beforehand on the stack and it will be de-allocated when you leave the scope\r\n\r\n- `run_with_initial_event()` is blocking, so you can safely pass all the state using `&` and you don't need to keep it alive\r\n\r\n- when you reach the final state, you stop the reactor, and so you basically unblock `run_with_initial_event()` and you leave the scope and cleanup everything\r\n\r\nHowever, currently it's not possible to do that with async code, since you don't find yourself in a blocking context where you can use RAII like above. That's the reason why we came up with rules involving shared pointers and passing them around to make sure we keep them safe until the final state.\r\n\r\nThis pull request aims to:\r\n\r\n1. make handling big async operations (like running a test or orchestration) as easy as it would be run them in a sync fashion using a simple main\r\n\r\n2. make sure that, even if `reactor->stop()` is not called because of a bug, the reactor is going to stop automatically when it has no further work do do\r\n\r\nTechnically, change 2 could have been implemented alone, but I tried that in #1293, only to discover that I needed to do other changes to make it work. So, overall, it would have been more complex to write two diffs than just a single one.\r\n\r\nImplementation\r\n==============\r\n\r\nTo achieve the above objectives, basically, what we do is that we require a specific reactor to be used for running big async operations.\r\n\r\nWe will run this reactor in a specific thread, and in a condition in which it's basically possible to replicate what you would do when writing a simple main. So, you can adopt more relaxed object rules (objective 1).\r\n\r\nAdditionally, we will change the way in which the reactor loops so that it leaves the loop when it is out of things to do (objective 2).\r\n\r\nFor example, we will be able to write:\r\n\r\n```C++\r\n/*\r\n * Hypothetical snippet from API for performing a single DNS query. Unlikely\r\n * we will implement something like that because it's a waste to use a reactor\r\n * only to perform this operation (at least currently I think so). Here only\r\n * to explain my point. More general than example above, because we need to\r\n * also show what it would entail to move around initial params.\r\n */\r\nusing namespace mk;\r\n// Note that Worker is an _internal_ API\r\nWorker::default_tasks_queue()->run_in_background_thread([\r\n    dns_server_address = std::move(dns_server_address),\r\n    dns_server_port,\r\n    dns_query_class = std::move(dns_query_class),\r\n    dns_query_type = std::move(dns_query_type),\r\n    dns_query = std::move(dns_query)\r\n]() {\r\n    Var<Reactor> reactor = Reactor::make();\r\n    // Note: nonexistent API here only to make a point\r\n    dns::Client client{reactor};\r\n    client.set_dns_server(dns_server_address, dns_server_port);\r\n    reactor->run_with_initial_event([&]() {\r\n        client.query(dns_query_class, dns_query_type, dns_query,\r\n                     [](Error error, dns::Message message) {\r\n                         // DO WHAT YOU NEED - MAYBE CALL A CALLBACK?\r\n                         reactor->stop();\r\n                     });\r\n    });\r\n});\r\n```\r\n\r\nLimitations in applicability\r\n============================\r\n\r\nThis enhancement basically binds an operation with a reactor.\r\n\r\nWhile this makes sense for big operations like tests, orchestration, or downloading resources, it would not make sense, of course, for smaller operations like a single DNS resolution or HTTP request.\r\n\r\nSo, the external API of smaller operations cannot be changed as a result of this pull request.\r\n\r\nHowever, the internal API can. For example, we can write an internal DNS API where we have a DNS client object that can be either used to implement the external DNS API or can be used by a specific test so that we do not need to allocate and deallocate a DNS client for each request we perform, rather we can reuse the same client, allocated using RAII as a sub-object of a nettest.\r\n\r\nAnd, when we are performing requests with such DNS client in the context of the larger DNS object, we can pass `this` to lambdas and generally be relaxed that the overall object is alive until end of test.\r\n\r\nSo, to summarize, this has an impact on internal implementation more than on external APIs. For external APIs, another solution should probably be found to achieve the same objective.\r\n\r\nOne solution I was thinking of was to allow to allocate objects in the context of the reactor, such that they are alive as long as the reactor loop is running and cancelled afterwards.\r\n\r\nThis is anyway something I will do in another pull request.\r\n\r\nDiscussion\r\n==========\r\n\r\nThis introduces the additional constraint that it binds the lifecycle of a reactor with the lifecycle of a big task or test.\r\n\r\nThis has more overhead than the current model where we use a single reactor loop to serve all the I/O in measurement-kit.\r\n\r\nYet, since measurement-kit is a measurement library, I don't see a use case where we're running many tasks in parallel.\r\n\r\nRather, I'd argue we want, in most cases, to run 1-2 tasks in parallel.\r\n\r\nAs such, for us nonblocking I/O is not a matter of scalability but rather a way to perform many async operations using a single thread.\r\n\r\nSo, I don't see binding a reactor to a specific operation as a disadvantage given the purpose of the library.\r\n\r\nMoreover, I see the following advantages:\r\n\r\n- with small code changes, it will become very easy to interrupt a running test, because it will just suffice to invoke `reactor->stop()` from another thread context\r\n\r\n- scheduling of big tasks or tests is already easier because we have a queue from which we pick and what cannot be served currently will stay in the queue until it's possible to serve it\r\n\r\n- writing internal code will become much simpler: it if works using a simple main() it will also work in async fashion\r\n\r\n- currently, we have an always-running thread for the reactor, while after this diff we will only have reactors running during a test or long task, which is perhaps more suitable to the requirements of mobile devices"},"comment":{"url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/comments/322925320","html_url":"https://github.com/measurement-kit/measurement-kit/pull/1297#issuecomment-322925320","issue_url":"https://api.github.com/repos/measurement-kit/measurement-kit/issues/1297","id":322925320,"user":{"login":"bassosimone","id":337298,"avatar_url":"https://avatars0.githubusercontent.com/u/337298?v=4","gravatar_id":"","url":"https://api.github.com/users/bassosimone","html_url":"https://github.com/bassosimone","followers_url":"https://api.github.com/users/bassosimone/followers","following_url":"https://api.github.com/users/bassosimone/following{/other_user}","gists_url":"https://api.github.com/users/bassosimone/gists{/gist_id}","starred_url":"https://api.github.com/users/bassosimone/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/bassosimone/subscriptions","organizations_url":"https://api.github.com/users/bassosimone/orgs","repos_url":"https://api.github.com/users/bassosimone/repos","events_url":"https://api.github.com/users/bassosimone/events{/privacy}","received_events_url":"https://api.github.com/users/bassosimone/received_events","type":"User","site_admin":false},"created_at":"2017-08-16T23:16:39Z","updated_at":"2017-08-16T23:16:39Z","body":"Okay, I should have fixed the build by now. This should now be ready for review, @hellais!"}}
{"actor":{"display_login":"bassosimone"}}