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

Proposed by Marcus Tomlinson on 2016-10-13
Status: Merged
Approved by: Marcus Tomlinson on 2016-10-13
Approved revision: 691
Merged at revision: 690
Proposed branch: lp:~marcustomlinson/unity-scopes-api/strict_idle_timeout
Merge into: lp:unity-scopes-api/devel
Diff against target: 46 lines (+6/-6)
3 files modified
doc/tutorial.dox (+2/-1)
src/scopes/internal/ScopeConfig.cpp (+3/-4)
test/gtest/scopes/internal/ScopeConfig/ScopeConfig_test.cpp (+1/-1)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scopes-api/strict_idle_timeout
Reviewer Review Type Date Requested Status
unity-api-1-bot continuous-integration Approve on 2016-10-13
Marcus Tomlinson (community) Approve on 2016-10-13
Michi Henning 2016-10-13 Pending
Review via email: mp+308346@code.launchpad.net

This proposal supersedes a proposal from 2016-10-12.

Commit message

Be much stricter on idle timeout (1s to 5min)

Description of the change

Be much stricter on idle timeout (1s to 5min). Not only are scopes currently allowed to set very very large timeouts, we seem to allow disabling of the timeout entirely (a value of -1)

To post a comment you must log in.
Michi Henning (michihenning) wrote : Posted in a previous version of this proposal

Looks good to me. Did we have problems with a scopes setting unreasonable values?

Not sure where the -1 problem is. The previous code complained if the value was < 0, meaning that you can have a scope that times out immediately. (Not that useful, I admit.)

I can't recall why we allowed maxint :-(

review: Approve
Marcus Tomlinson (marcustomlinson) wrote : Posted in a previous version of this proposal

> Looks good to me. Did we have problems with a scopes setting unreasonable
> values?

I don't know of a particular case where a scope is doing this, but that's part of the concern I suppose. Feels like a bit of a security risk allowing scopes to essentially act like daemons this way.

>
> Not sure where the -1 problem is. The previous code complained if the value
> was < 0, meaning that you can have a scope that times out immediately. (Not
> that useful, I admit.)

If you look at the diff we allowed ((>=0 && <= max) || -1)

>
> I can't recall why we allowed maxint :-(

Me neither.

Michi Henning (michihenning) wrote : Posted in a previous version of this proposal

> I don't know of a particular case where a scope is doing this, but that's part
> of the concern I suppose. Feels like a bit of a security risk allowing scopes
> to essentially act like daemons this way.

Back then, we knew little about how scopes would be used and what they might be doing. I suspect that's why it ended up being so liberal.

> If you look at the diff we allowed ((>=0 && <= max) || -1)

Ah, yes. I didn't read it closely enough. Anyway, I think it's fine a limit it to five minutes. If that breaks some scope, we'll find out soon enough.

Marcus Tomlinson (marcustomlinson) wrote :

Targeted trunk by mistake. Re-approving for merge to devel

review: Approve
unity-api-1-bot (unity-api-1-bot) wrote :
review: Needs Fixing (continuous-integration)
unity-api-1-bot (unity-api-1-bot) wrote :
review: Needs Fixing (continuous-integration)
691. By Marcus Tomlinson on 2016-10-13

Oops, fix ScopeConfig_test.cpp

unity-api-1-bot (unity-api-1-bot) wrote :

PASSED: Continuous integration, rev:691
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/49/
Executed test runs:
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build/882
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-0-fetch/889
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/695
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=vivid+overlay/695/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/695
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=xenial+overlay/695/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/695
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=amd64,release=yakkety/695/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/695
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=vivid+overlay/695/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/695
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=xenial+overlay/695/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/695
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=armhf,release=yakkety/695/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/695
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=vivid+overlay/695/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/695
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=xenial+overlay/695/artifact/output/*zip*/output.zip
    SUCCESS: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/695
        deb: https://jenkins.canonical.com/unity-api-1/job/build-2-binpkg/arch=i386,release=yakkety/695/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://jenkins.canonical.com/unity-api-1/job/lp-unity-scopes-api-ci/49/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'doc/tutorial.dox'
2--- doc/tutorial.dox 2016-06-15 09:16:47 +0000
3+++ doc/tutorial.dox 2016-10-13 11:57:31 +0000
4@@ -1560,7 +1560,8 @@
5
6 The `IdleTimeout` key controls how long a scope can remain idle before it is told to stop by the registry (or
7 killed if it does not stop within 4 seconds). The default idle timeout is 40 seconds, meaning that a
8-scope will be told to stop if no query was sent to it for that amount of time.
9+scope will be told to stop if no query was sent to it for that amount of time. Only values in the range 1 to
10+300 seconds are accepted.
11
12 `ResultTtl` determines how long results should be cached by the UI before they are considered "stale"
13 and should be refreshed. `None` indicates that results remain valid indefinitely; `Small` indicates
14
15=== modified file 'src/scopes/internal/ScopeConfig.cpp'
16--- src/scopes/internal/ScopeConfig.cpp 2016-03-31 09:27:33 +0000
17+++ src/scopes/internal/ScopeConfig.cpp 2016-10-13 11:57:31 +0000
18@@ -173,12 +173,11 @@
19
20 idle_timeout_ = get_optional_int(scope_config_group, idle_timeout_key, DFLT_SCOPE_IDLE_TIMEOUT);
21
22- // Negative values and values greater than max int (once multiplied by 1000 (s to ms)) are illegal
23- const int max_idle_timeout = std::numeric_limits<int>::max() / 1000;
24- if ((idle_timeout_ < 0 || idle_timeout_ > max_idle_timeout) && idle_timeout_ != -1)
25+ // Values less than 1s and greater than 300s are illegal
26+ if (idle_timeout_ < 1 || idle_timeout_ > 300)
27 {
28 throw_ex("Illegal value (" + std::to_string(idle_timeout_) + ") for " + idle_timeout_key +
29- ": value must be >= 0 and <= " + std::to_string(max_idle_timeout));
30+ ": value must be >= 1 and <= 300");
31 }
32
33 results_ttl_type_ = ScopeMetadata::ResultsTtlType::None;
34
35=== modified file 'test/gtest/scopes/internal/ScopeConfig/ScopeConfig_test.cpp'
36--- test/gtest/scopes/internal/ScopeConfig/ScopeConfig_test.cpp 2015-05-14 02:50:35 +0000
37+++ test/gtest/scopes/internal/ScopeConfig/ScopeConfig_test.cpp 2016-10-13 11:57:31 +0000
38@@ -156,7 +156,7 @@
39 catch(ConfigException const& e)
40 {
41 boost::regex r("unity::scopes::ConfigException: \".*\": Illegal value \\(-2\\) for IdleTimeout: "
42- "value must be >= 0 and <= [0-9]+");
43+ "value must be >= 1 and <= [0-9]+");
44 EXPECT_TRUE(boost::regex_match(e.what(), r));
45 }
46 }

Subscribers

People subscribed via source and target branches

to all changes: