Addition of an option to throttle the signing requests. #159
Conversation
Largely added because even with "--mdop 1" I still periodically get errors from timestamp servers.
There was a problem hiding this comment.
In theory I like this idea, but I do not think AzureSign.Core should be changed. This seems more like a change with how AzureSignTool interacts with AzureSign.Core. AzureSign.Core's job is to just sign things, unaware of its surroundings.
I would consider moving the throttling and tracking to somewhere here:
I'll shift the changes to the loop. Makes the changes a bit simpler too, really. |
|
Is this possible to validate this PR? I also experience throttling with my timestamp provider. |
| { | ||
| DateTime goTime = _lastSigningOperation.AddSeconds(SigningThrottle.Value); | ||
|
|
||
| if (goTime >= DateTime.Now) |
There was a problem hiding this comment.
I have some concerns around this since DateTime.Now is not monotonic - for example, a Daylight Saving Time. Let's say the _lastSigningOperation is at 1:59 AM. Day Light Savings kicks in at 2:00 AM, and jumps forward to 3:00 AM. This will violate the throttle.
Using DateTimeOffset.UtcNow probably mitigates most of those concerns... except for things like clock correction. I think that is a tradeoff I am willing to accept. A "true" monotonic clock source is probably overkill for this purpose.
Can we re-work this to use DateTimeOffset.UtcNow?
There was a problem hiding this comment.
Could possibly use System.Threading.PeriodicTimer instead, which was added at some point, but I'd need to check the version availability for it. You just call the wait method on the class; it'll return immediately if there's already been one or more ticks since the method was last called, or it will wait until there is one and then return.
| @@ -96,7 +99,9 @@ internal sealed class SignCommand | |||
| [Argument(0, "file", "The path to the file.")] | |||
| public string[] Files { get; set; } = Array.Empty<string>(); | |||
|
|
|||
| private static DateTime _lastSigningOperation; | |||
There was a problem hiding this comment.
Does this need to be static? Does it even need to be a field, or can it be part of the state in Parallel.ForEach?
There was a problem hiding this comment.
It's been a while since I wrote this and I'll have to double check whether throttling limits it you to a single request thread. I have a feeling I made it so that it did; if I did then there's not really any need for it to be static and I'll pull it in to the state.
Even with the -mdop option set to 1 I still periodically get errors from timestamp servers, so this option attempts to add another tool to the chest to prevent this.