Merge lp:~marcustomlinson/unity-scopes-api/fix_debug_mode_locate_timeout into lp:unity-scopes-api/devel

[Marcus Tomlinson]

Allow 30s for locate() to complete when requesting a scope's debug_mode() status.

[Marcus Tomlinson (marcustomlinson)]

The main fix here is:

26 + auto future = mw_base()->twoway_pool()->submit([&] { return this->invoke_twoway_(request_builder, -1, 30000); });

We need to actually use debug timeouts when requesting whether a scope is in debug mode :P

The problem is the initial locate() call that happens implicitly when making any request to a scope. This call was using the normal timeout constraint. Issue there is if a scope actually is in debug mode it takes longer to start up, hence that locate() call times out.

In the situation where a scope is not in debug mode but is starting up too slowly, it will not take 30s to timeout. In this case the registry's own "Process.Timeout" will kick in (which is configured according to the debug_mode status of each scope as it is added), causing the timeout to occur at a regular timeout for regular scope, and at an extended timeout for "debug mode" scopes.

[Paweł Stołowski (stolowski)]

Looks good to me. +1

review: Approve

[Michi Henning (michihenning)]

Looks reasonable. Not totally fond of the hard-wired constant there.

I'm not going to hold up this MR because of it. But should we make that a config item? If there is more than one place where that number is used, we probably should... If so, could you open a separate bug please?

review: Approve

[Paweł Stołowski (stolowski)]

Setting to needs review at the top level; Benjamin will top approve after verification.

[Marcus Tomlinson (marcustomlinson)]

> Looks reasonable. Not totally fond of the hard-wired constant there.
>
> I'm not going to hold up this MR because of it. But should we make that a
> config item? If there is more than one place where that number is used, we
> probably should... If so, could you open a separate bug please?

Yeah its a good point. Was just thinking about where to put those. I'm thinking a "Debug.Process.Timeout" in the registry config, and a "Debug.Locate.Timeout" in the ZMQ config. Sound good?

[Michi Henning (michihenning)]

Sounds good. Please remember to update the doxygen doc and/or CONFIGFILES as appropriate. No great urgency here. But, if we can't do this right away, please open a bug so we won't forget.

[Marcus Tomlinson (marcustomlinson)]

> Sounds good. Please remember to update the doxygen doc and/or CONFIGFILES as
> appropriate. No great urgency here. But, if we can't do this right away,
> please open a bug so we won't forget.

Bug created: https://bugs.launchpad.net/ubuntu/+source/unity-scopes-api/+bug/1469105

[Michi Henning (michihenning)]

Thank you Sir! :-)

Michi.

On 26 Jun 2015, at 20:21 , Marcus Tomlinson <email address hidden> wrote:

>> Sounds good. Please remember to update the doxygen doc and/or CONFIGFILES as
>> appropriate. No great urgency here. But, if we can't do this right away,
>> please open a bug so we won't forget.
>
> Bug created: https://bugs.launchpad.net/ubuntu/+source/unity-scopes-api/+bug/1469105
> --
> https://code.launchpad.net/~marcustomlinson/unity-scopes-api/fix_debug_mode_locate_timeout/+merge/263074
> You are reviewing the proposed merge of lp:~marcustomlinson/unity-scopes-api/fix_debug_mode_locate_timeout into lp:unity-scopes-api/devel.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:602
http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-ci/1475/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-amd64-ci/16
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-armhf-ci/16
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-armhf-ci/16/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-api-devel-wily-i386-ci/16

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-api-devel-ci/1475/rebuild

review: Approve (continuous-integration)

[Marcus Tomlinson (marcustomlinson)]

Benjamin has confirmed at least that the new debug timeouts do work, however, we're gonna hold off merging this MP until some other debugging issues are resolved.

[Marcus Tomlinson (marcustomlinson)]

> Benjamin has confirmed at least that the new debug timeouts do work, however,
> we're gonna hold off merging this MP until some other debugging issues are
> resolved.

Permission granted to top approve by Benjamin. Thanks!

=== modified file 'scoperegistry/scoperegistry.cpp'
--- scoperegistry/scoperegistry.cpp	2015-04-10 03:55:25 +0000
+++ scoperegistry/scoperegistry.cpp	2015-06-26 09:03:50 +0000
@@ -423,10 +423,10 @@
                 scope_dir.filename().native();
     }
 
-    // Check if this scope has requested debug mode, if so, set the process timeout to 15s
+    // Check if this scope has requested debug mode, if so, set the process timeout to 30s
     if (sc.debug_mode())
     {
-        exec_data.timeout_ms = 15000;
+        exec_data.timeout_ms = 30000;
     }
     else
     {

=== modified file 'src/scopes/internal/zmq_middleware/ZmqScope.cpp'
--- src/scopes/internal/zmq_middleware/ZmqScope.cpp	2015-05-29 12:02:54 +0000
+++ src/scopes/internal/zmq_middleware/ZmqScope.cpp	2015-06-26 09:03:50 +0000
@@ -325,7 +325,7 @@
         capnp::MallocMessageBuilder request_builder;
         make_request_(request_builder, "debug_mode");
 
-        auto future = mw_base()->twoway_pool()->submit([&] { return this->invoke_twoway_(request_builder); });
+        auto future = mw_base()->twoway_pool()->submit([&] { return this->invoke_twoway_(request_builder, -1, 30000); });
         auto out_params = future.get();
         auto response = out_params.reader->getRoot<capnproto::Response>();
         throw_if_runtime_exception(response);
@@ -345,10 +345,10 @@
 ZmqObjectProxy::TwowayOutParams ZmqScope::invoke_scope_(capnp::MessageBuilder& in_params, int64_t timeout)
 {
     // Check if this scope has requested debug mode, if so, disable two-way timeout and set
-    // locate timeout to 15s.
+    // locate timeout to 30s.
     if (debug_mode())
     {
-        return this->invoke_twoway_(in_params, -1, 15000);
+        return this->invoke_twoway_(in_params, -1, 30000);
     }
     return this->invoke_twoway_(in_params, timeout);
 }