EES-4879 Adding a blanket 30s timeout across all Public API endpoints #5137
Conversation
| options.DefaultPolicy = | ||
| new RequestTimeoutPolicy | ||
| { | ||
| Timeout = TimeSpan.FromMilliseconds(30000), |
There was a problem hiding this comment.
I don't knwo if it's worth making this configurable (with a 30s fallback), and set to something like 500ms in our integration tests? That way, we can register a test controller that delays for 1s and then we can test the response.
There was a problem hiding this comment.
Good idea, I'll make that change!
| @@ -19,5 +19,6 @@ | |||
| }, | |||
| "DataFiles": { | |||
| "BasePath": "data/public-api-data" | |||
| } | |||
| }, | |||
There was a problem hiding this comment.
I guess with the fallback, you don't really need this value here.
There was a problem hiding this comment.
No that's true! I thought I'd leave it there for discoverability. But I'll remove it :)
jack-hive
force-pushed
the
EES-4879
branch
3 times, most recently
from
0aa941d to
8843c42
Compare
| options.DefaultPolicy = | ||
| new RequestTimeoutPolicy | ||
| { | ||
| Timeout = TimeSpan.FromMilliseconds(configuration.GetValue<int?>("RequestTimeoutMilliseconds") ?? 30000), |
There was a problem hiding this comment.
Might be good for us to extract out an AppSettingsOptions class to house this configuration. We already do this in other parts of the service and would give some type-safety over this.
This could then be re-used in the tests as well.
There was a problem hiding this comment.
I was thinking of doing this, but then thought since it's just a one-off setting and I couldn't envisage us nesting any other options beneath the 'timeout' section as such, I thought I'd just simplify it by removing the need for the extra class. But I agree, it might be slightly better to do this. I've made a change :)
| public class RequestTimeoutOptions | ||
| { | ||
| public const string Section = "RequestTimeouts"; | ||
|
|
||
| public int? RequestTimeoutMilliseconds { get; init; } |
There was a problem hiding this comment.
Given we've opted to put this in a RequestTimeout section, we could simplify this key to just TimeoutMilliseconds.
I was also wondering if you could just put the default value (30000) in here as well? That way we don't need to put the default in Startup. Not entirely sure if this works as we'd expect or not though (would suggest checking).
There was a problem hiding this comment.
Renamed :)
I tested the default, but it doesn't work unfortunately :(
|
|
||
| public class RequestTimeoutOptions | ||
| { | ||
| public const string Section = "RequestTimeouts"; |
There was a problem hiding this comment.
This should be RequestTimeout to align with the class
I have no idea how this happened. I'm pretty sure I used F2 to rename the variable everywhere!. But my Visual Studio has been playing up a lot lately with detecting renames. I might have to get into the habit of running tests again even after something as simple as a rename :P |
…n't affect the other tests
This PR adds a 30 second
504timeout to all Public API endpoints.We also have a change to document these
504timeout responses in the swagger documentation across all endpoints.