EdgeAgent: Change twin refresh timer logic to loop #799
Conversation
edge-util/src/Microsoft.Azure.Devices.Edge.Util/PeriodicTask.cs
Outdated
Show resolved
Hide resolved
edge-util/src/Microsoft.Azure.Devices.Edge.Util/PeriodicTask.cs
Outdated
Show resolved
Hide resolved
edge-util/src/Microsoft.Azure.Devices.Edge.Util/PeriodicTask.cs
Outdated
Show resolved
Hide resolved
edge-util/test/Microsoft.Azure.Devices.Edge.Util.Test/PeriodicTaskTest.cs
Outdated
Show resolved
Hide resolved
edge-util/test/Microsoft.Azure.Devices.Edge.Util.Test/PeriodicTaskTest.cs
Show resolved
Hide resolved
| @@ -64,8 +64,7 @@ public class EdgeAgentConnection : IEdgeAgentConnection | |||
| this.reportedProperties = Option.None<TwinCollection>(); | |||
| this.deviceClient = Option.None<IModuleClient>(); | |||
| this.retryStrategy = Preconditions.CheckNotNull(retryStrategy, nameof(retryStrategy)); | |||
| this.refreshTimer = new Timer(refreshConfigFrequency.TotalMilliseconds); | |||
| this.refreshTimer.Elapsed += (_, __) => this.RefreshTimerElapsed(); | |||
| this.refreshTwinTask = new PeriodicTask(this.RefreshTwinAsync, refreshConfigFrequency, refreshConfigFrequency, Events.Log, "refresh twin config"); | |||
There was a problem hiding this comment.
this.RefreshTwinAsync can be called from this PeriodicTask, CreateAndInitDeviceClient, OnConnectionStatusChanged, and OnDesiredPropertiesUpdated. First periodic task doesn't call twinLock.LockAsync before calling RefreshTwinAsync; may move twinLock.LockAsync statement inside RefreshTwinAsync method.
Besides that I concern this may run too often in some cases, for example if it triggered by OnDesiredPropertiesUpdated, and then periodictask also trigger right after; I would suggest to reset timer inside periodictask after RefreshTwinAsync called.
There was a problem hiding this comment.
yes, we had the reset logic earlier... but it is non-trivial to do it with a loop and I wanted to avoid the complexity that comes with it. This also makes it easier to track logs to see if the periodic task is working properly.
The logic in Edge agent to refresh its twin periodically was based on a timer, and seemed to not work in some customer scenarios. So converting the logic to be loop based, with additional checks to make sure it works as expected at all times.