Code review comment for lp:~marcustomlinson/unity-scopes-api/disable_timeouts_in_debug_mode

Revision history for this message
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.

« Back to merge proposal