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

Proposed by Marcus Tomlinson on 2015-07-01
Status: Merged
Approved by: Michi Henning on 2015-07-03
Approved revision: 609
Merged at revision: 608
Proposed branch: lp:~marcustomlinson/unity-scopes-api/disable_timeouts_in_debug_mode
Merge into: lp:unity-scopes-api/devel
Prerequisite: lp:~marcustomlinson/unity-scopes-api/revert_debug_timeout_config_params
Diff against target: 131 lines (+39/-19)
5 files modified
include/unity/scopes/internal/zmq_middleware/ZmqObjectProxy.h (+3/-1)
scoperegistry/scoperegistry.cpp (+2/-2)
src/scopes/internal/RegistryObject.cpp (+11/-3)
src/scopes/internal/zmq_middleware/ZmqObject.cpp (+9/-9)
src/scopes/internal/zmq_middleware/ZmqScope.cpp (+14/-4)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scopes-api/disable_timeouts_in_debug_mode
Reviewer Review Type Date Requested Status
Michi Henning (community) Approve on 2015-07-03
PS Jenkins bot (community) continuous-integration Approve on 2015-07-01
Paweł Stołowski 2015-07-01 Needs Information on 2015-07-01
Review via email: mp+263500@code.launchpad.net

Commit message

Disable process and locate timeouts completely in debug mode

To post a comment you must log in.
Paweł Stołowski (stolowski) wrote :

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

This change looks a bit suspicious, is this intended? In full context it is:

    if (!debug_mode_)
    {
        capnp::MallocMessageBuilder request_builder;
        make_request_(request_builder, "debug_mode");

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

Which, if I understand it correctly, means we don't apply timeout when running normally (i.e. not in debug mode)?

review: Needs Information
Marcus Tomlinson (marcustomlinson) wrote :

> 102 - auto future = mw_base()->twoway_pool()->submit([&] { return
> this->invoke_twoway_(request_builder, -1, 30000); });
> 103 + auto future = mw_base()->twoway_pool()->submit([&] { return
> this->invoke_twoway_(request_builder, -1, -1); });
>
> This change looks a bit suspicious, is this intended? In full context it is:
>
> if (!debug_mode_)
> {
> capnp::MallocMessageBuilder request_builder;
> make_request_(request_builder, "debug_mode");
>
> auto future = mw_base()->twoway_pool()->submit([&] { return
> this->invoke_twoway_(request_builder, -1, -1); });
>
> Which, if I understand it correctly, means we don't apply timeout when running
> normally (i.e. not in debug mode)?

So the timeout we're disabling here is the locate() call that happens implicitly when making any request. It goes:
    1. reg->locate(scope) - first ensure the scope is running
    2. scope->debug_mode() - make the actual request

The issue there is if a scope actually is in debug mode and it takes long to start up, that implicit locate() call will time out prematurely.

In the situation where a scope is not in debug mode but starts up slowly (E.g. has a breakpoint in start()), the registry's own "Process.Timeout" will kick in (which is configured according to the "DebugMode" state of each scope config as they are added). Meaning, that implicit locate() call will actually still timeout due to the registry's "Process.Timeout" constraint when a regular scope takes too long to start.

Make sense?

609. By Marcus Tomlinson on 2015-07-01

Added comment on why we disable locate() timeout on debug_mode request.
Also fixed my mistake of disabling the two-way timeout there aswell (oops).

Marcus Tomlinson (marcustomlinson) wrote :

> > 102 - auto future = mw_base()->twoway_pool()->submit([&] { return
> > this->invoke_twoway_(request_builder, -1, 30000); });
> > 103 + auto future = mw_base()->twoway_pool()->submit([&] { return
> > this->invoke_twoway_(request_builder, -1, -1); });
> >
> > This change looks a bit suspicious, is this intended? In full context it is:
> >
> > if (!debug_mode_)
> > {
> > capnp::MallocMessageBuilder request_builder;
> > make_request_(request_builder, "debug_mode");
> >
> > auto future = mw_base()->twoway_pool()->submit([&] { return
> > this->invoke_twoway_(request_builder, -1, -1); });
> >
> > Which, if I understand it correctly, means we don't apply timeout when
> running
> > normally (i.e. not in debug mode)?
>
> So the timeout we're disabling here is the locate() call that happens
> implicitly when making any request. It goes:
> 1. reg->locate(scope) - first ensure the scope is running
> 2. scope->debug_mode() - make the actual request
>
> The issue there is if a scope actually is in debug mode and it takes long to
> start up, that implicit locate() call will time out prematurely.
>
> In the situation where a scope is not in debug mode but starts up slowly (E.g.
> has a breakpoint in start()), the registry's own "Process.Timeout" will kick
> in (which is configured according to the "DebugMode" state of each scope
> config as they are added). Meaning, that implicit locate() call will actually
> still timeout due to the registry's "Process.Timeout" constraint when a
> regular scope takes too long to start.
>
> Make sense?

Sorry, that was a little confusing (even to me) :P Add a comment to the code.

Michi Henning (michihenning) wrote :

Looks good to me. Thanks for the comment, that really made a huge difference!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'include/unity/scopes/internal/zmq_middleware/ZmqObjectProxy.h'
2--- include/unity/scopes/internal/zmq_middleware/ZmqObjectProxy.h 2015-02-09 02:23:22 +0000
3+++ include/unity/scopes/internal/zmq_middleware/ZmqObjectProxy.h 2015-07-01 16:48:22 +0000
4@@ -91,8 +91,10 @@
5
6 TwowayOutParams invoke_twoway_(capnp::MessageBuilder& request);
7 TwowayOutParams invoke_twoway_(capnp::MessageBuilder& request,
8+ int64_t twoway_timeout);
9+ TwowayOutParams invoke_twoway_(capnp::MessageBuilder& request,
10 int64_t twoway_timeout,
11- int64_t locate_timeout = -1);
12+ int64_t locate_timeout);
13
14 private:
15 TwowayOutParams invoke_twoway__(capnp::MessageBuilder& request, int64_t timeout);
16
17=== modified file 'scoperegistry/scoperegistry.cpp'
18--- scoperegistry/scoperegistry.cpp 2015-07-01 10:59:49 +0000
19+++ scoperegistry/scoperegistry.cpp 2015-07-01 16:48:22 +0000
20@@ -423,10 +423,10 @@
21 scope_dir.filename().native();
22 }
23
24- // Check if this scope has requested debug mode, if so, set the process timeout to 30s
25+ // Check if this scope has requested debug mode, if so, disable process timeout
26 if (sc.debug_mode())
27 {
28- exec_data.timeout_ms = 30000;
29+ exec_data.timeout_ms = -1;
30 }
31 else
32 {
33
34=== modified file 'src/scopes/internal/RegistryObject.cpp'
35--- src/scopes/internal/RegistryObject.cpp 2015-04-23 10:11:51 +0000
36+++ src/scopes/internal/RegistryObject.cpp 2015-07-01 16:48:22 +0000
37@@ -736,9 +736,17 @@
38
39 bool RegistryObject::ScopeProcess::wait_for_state(std::unique_lock<std::mutex>& lock, ProcessState state) const
40 {
41- return state_change_cond_.wait_for(lock,
42- std::chrono::milliseconds(exec_data_.timeout_ms),
43- [this, &state]{return state_ == state;});
44+ if (exec_data_.timeout_ms == -1)
45+ {
46+ state_change_cond_.wait(lock, [this, &state]{return state_ == state;});
47+ return true;
48+ }
49+ else
50+ {
51+ return state_change_cond_.wait_for(lock,
52+ std::chrono::milliseconds(exec_data_.timeout_ms),
53+ [this, &state]{return state_ == state;});
54+ }
55 }
56
57 void RegistryObject::ScopeProcess::kill(std::unique_lock<std::mutex>& lock)
58
59=== modified file 'src/scopes/internal/zmq_middleware/ZmqObject.cpp'
60--- src/scopes/internal/zmq_middleware/ZmqObject.cpp 2015-01-06 01:05:48 +0000
61+++ src/scopes/internal/zmq_middleware/ZmqObject.cpp 2015-07-01 16:48:22 +0000
62@@ -190,7 +190,13 @@
63
64 ZmqObjectProxy::TwowayOutParams ZmqObjectProxy::invoke_twoway_(capnp::MessageBuilder& request)
65 {
66- return invoke_twoway_(request, timeout_);
67+ return invoke_twoway_(request, timeout_, mw_base()->locate_timeout());
68+}
69+
70+ZmqObjectProxy::TwowayOutParams ZmqObjectProxy::invoke_twoway_(capnp::MessageBuilder& request,
71+ int64_t twoway_timeout)
72+{
73+ return invoke_twoway_(request, twoway_timeout, mw_base()->locate_timeout());
74 }
75
76 ZmqObjectProxy::TwowayOutParams ZmqObjectProxy::invoke_twoway_(capnp::MessageBuilder& request,
77@@ -211,14 +217,8 @@
78 try
79 {
80 ObjectProxy new_proxy;
81- if (locate_timeout != -1)
82- {
83- new_proxy = registry_proxy->locate(identity(), locate_timeout);
84- }
85- else
86- {
87- new_proxy = registry_proxy->locate(identity());
88- }
89+ new_proxy = registry_proxy->locate(identity(), locate_timeout);
90+
91 // update our proxy with the newly received data
92 // (we need to first store values in local variables outside of the mutex,
93 // otherwise we will deadlock on the following ZmqObjectProxy methods)
94
95=== modified file 'src/scopes/internal/zmq_middleware/ZmqScope.cpp'
96--- src/scopes/internal/zmq_middleware/ZmqScope.cpp 2015-07-01 10:59:49 +0000
97+++ src/scopes/internal/zmq_middleware/ZmqScope.cpp 2015-07-01 16:48:22 +0000
98@@ -325,7 +325,18 @@
99 capnp::MallocMessageBuilder request_builder;
100 make_request_(request_builder, "debug_mode");
101
102- auto future = mw_base()->twoway_pool()->submit([&] { return this->invoke_twoway_(request_builder, -1, 30000); });
103+ // When making any two-way request there is an implicit locate() call made to the registry to first ensure that
104+ // the scope is actually running - I.e. 1. reg->locate(scope), 2. scope->request().
105+
106+ // Here we are trying to determine whether the target scope is in debug mode, hence we cannot assume upfront
107+ // that it will start up within the regular timeout constraint. Therefore we need to disable the locate()
108+ // timeout here to allow enough time for debugged scopes to reply.
109+
110+ // In the situation where a scope is not in debug mode but starts up slowly, the registry's own
111+ // "Process.Timeout" will kick in, meaning that the implicit locate() call will return at a regular timeout for
112+ // regular scopes, and will not timeout for debugged scopes.
113+
114+ auto future = mw_base()->twoway_pool()->submit([&] { return this->invoke_twoway_(request_builder, timeout(), -1); });
115 auto out_params = future.get();
116 auto response = out_params.reader->getRoot<capnproto::Response>();
117 throw_if_runtime_exception(response);
118@@ -344,11 +355,10 @@
119
120 ZmqObjectProxy::TwowayOutParams ZmqScope::invoke_scope_(capnp::MessageBuilder& in_params, int64_t timeout)
121 {
122- // Check if this scope has requested debug mode, if so, disable two-way timeout and set
123- // locate timeout to 30s.
124+ // Check if this scope has requested debug mode, if so, disable two-way and locate timeouts.
125 if (debug_mode())
126 {
127- return this->invoke_twoway_(in_params, -1, 30000);
128+ return this->invoke_twoway_(in_params, -1, -1);
129 }
130 return this->invoke_twoway_(in_params, timeout);
131 }

Subscribers

People subscribed via source and target branches

to all changes: