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

Proposed by Marcus Tomlinson on 2016-10-12
Status: Superseded
Proposed branch: lp:~marcustomlinson/unity-scopes-api/strict_idle_timeout
Merge into: lp:unity-scopes-api
Diff against target: 33 lines (+5/-5)
2 files modified
doc/tutorial.dox (+2/-1)
src/scopes/internal/ScopeConfig.cpp (+3/-4)
To merge this branch: bzr merge lp:~marcustomlinson/unity-scopes-api/strict_idle_timeout
Reviewer Review Type Date Requested Status
Michi Henning (community) 2016-10-12 Approve on 2016-10-12
Review via email: mp+308251@code.launchpad.net

This proposal has been superseded by a proposal from 2016-10-13.

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 :

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 :

> 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 :

> 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.

691. By Marcus Tomlinson on 2016-10-13

Oops, fix ScopeConfig_test.cpp

Unmerged revisions

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-12 12:46:20 +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-12 12:46:20 +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;

Subscribers

People subscribed via source and target branches

to all changes: