Merge pull request #2591 from lonelam/fixstat

FIX: some crash about thread-safe in statistics

shadowsocks-csharp/Controller/Service/AvailabilityStatistics.cs

@@ -71,9 +71,8 @@ namespace Shadowsocks.Controller
         //tasks
         private readonly TimeSpan _delayBeforeStart = TimeSpan.FromSeconds(1);
         private readonly TimeSpan _retryInterval = TimeSpan.FromMinutes(2);
-        private Timer _recorder; //analyze and save cached records to RawStatistics and filter records
         private TimeSpan RecordingInterval => TimeSpan.FromMinutes(Config.DataCollectionMinutes);
-        private Timer _speedMonior;
+        private Timer _perSecondTimer; //analyze and save cached records to RawStatistics and filter records
         private readonly TimeSpan _monitorInterval = TimeSpan.FromSeconds(1);
         //private Timer _writer; //write RawStatistics to file
         //private readonly TimeSpan _writingInterval = TimeSpan.FromMinutes(1);
@@ -99,14 +98,15 @@ namespace Shadowsocks.Controller
             {
                 if (Config.StatisticsEnabled)
                 {
-                    StartTimerWithoutState(ref _recorder, Run, RecordingInterval);
                     LoadRawStatistics();
-                    StartTimerWithoutState(ref _speedMonior, UpdateSpeed, _monitorInterval);
+                    if (_perSecondTimer == null)
+                    {
+                        _perSecondTimer = new Timer(OperationsPerSecond, new Counter(), _delayBeforeStart, TimeSpan.FromSeconds(1));
+                    }
                 }
                 else
                 {
-                    _recorder?.Dispose();
-                    _speedMonior?.Dispose();
+                    _perSecondTimer?.Dispose();
                 }
             }
             catch (Exception e)
@@ -115,15 +115,26 @@ namespace Shadowsocks.Controller
             }
         }
 
-        private void StartTimerWithoutState(ref Timer timer, TimerCallback callback, TimeSpan interval)
+        private void OperationsPerSecond(object state)
         {
-            if (timer?.Change(_delayBeforeStart, interval) == null)
+            lock(state)
             {
-                timer = new Timer(callback, null, _delayBeforeStart, interval);
+                var counter = state as Counter;
+                if (counter.count % _monitorInterval.TotalSeconds == 0)
+                {
+                    UpdateSpeed();
+                }
+
+                if (counter.count % RecordingInterval.TotalSeconds == 0)
+                {
+                    Run();
+                }
+
+                counter.count++;
             }
         }
 
-        private void UpdateSpeed(object _)
+        private void UpdateSpeed()
         {
             foreach (var kv in _inOutBoundRecords)
             {
@@ -137,6 +148,7 @@ namespace Shadowsocks.Controller
                 var inboundSpeed = GetSpeedInKiBPerSecond(inboundDelta, _monitorInterval.TotalSeconds);
                 var outboundSpeed = GetSpeedInKiBPerSecond(outboundDelta, _monitorInterval.TotalSeconds);
 
+                // not thread safe
                 var inR = _inboundSpeedRecords.GetOrAdd(id, (k) => new List<int>());
                 var outR = _outboundSpeedRecords.GetOrAdd(id, (k) => new List<int>());
 
@@ -155,7 +167,7 @@ namespace Shadowsocks.Controller
             _latencyRecords.Clear();
         }
 
-        private void Run(object _)
+        private void Run()
         {
             UpdateRecords();
             Reset();
@@ -165,35 +177,47 @@ namespace Shadowsocks.Controller
         {
             var records = new Dictionary<string, StatisticsRecord>();
             UpdateRecordsState state = new UpdateRecordsState();
-            state.counter = _controller.GetCurrentConfiguration().configs.Count;
-            foreach (var server in _controller.GetCurrentConfiguration().configs)
-            {
-                var id = server.Identifier();
-                List<int> inboundSpeedRecords = null;
-                List<int> outboundSpeedRecords = null;
-                List<int> latencyRecords = null;
-                _inboundSpeedRecords.TryGetValue(id, out inboundSpeedRecords);
-                _outboundSpeedRecords.TryGetValue(id, out outboundSpeedRecords);
-                _latencyRecords.TryGetValue(id, out latencyRecords);
-                StatisticsRecord record = new StatisticsRecord(id, inboundSpeedRecords, outboundSpeedRecords, latencyRecords);
-                /* duplicate server identifier */
-                if (records.ContainsKey(id))
-                    records[id] = record;
-                else
-                    records.Add(id, record);
-                if (Config.Ping)
+            int serverCount = _controller.GetCurrentConfiguration().configs.Count;
+            state.counter = serverCount;
+            bool isPing = Config.Ping;
+            for (int i = 0; i < serverCount; i++)
+            {
+                try
                 {
-                    MyPing ping = new MyPing(server, Repeat);
-                    ping.Completed += ping_Completed;
-                    ping.Start(new PingState { state = state, record = record });
+                    var server = _controller.GetCurrentConfiguration().configs[i];
+                    var id = server.Identifier();
+                    List<int> inboundSpeedRecords = null;
+                    List<int> outboundSpeedRecords = null;
+                    List<int> latencyRecords = null;
+                    _inboundSpeedRecords.TryGetValue(id, out inboundSpeedRecords);
+                    _outboundSpeedRecords.TryGetValue(id, out outboundSpeedRecords);
+                    _latencyRecords.TryGetValue(id, out latencyRecords);
+                    StatisticsRecord record = new StatisticsRecord(id, inboundSpeedRecords, outboundSpeedRecords, latencyRecords);
+                    /* duplicate server identifier */
+                    if (records.ContainsKey(id))
+                        records[id] = record;
+                    else
+                        records.Add(id, record);
+                    if (isPing)
+                    {
+                        // FIXME: on ping completed, every thing could be asynchrously changed.
+                        // focus on: Config/ RawStatistics
+                        MyPing ping = new MyPing(server, Repeat);
+                        ping.Completed += ping_Completed;
+                        ping.Start(new PingState { state = state, record = record });
+                    }
+                    else if (!record.IsEmptyData())
+                    {
+                        AppendRecord(id, record);
+                    }
                 }
-                else if (!record.IsEmptyData())
+                catch (Exception e)
                 {
-                    AppendRecord(id, record);
+                    Logging.Debug("config changed asynchrously, just ignore this server");
                 }
             }
 
-            if (!Config.Ping)
+            if (!isPing)
             {
                 Save();
                 FilterRawStatistics();
@@ -278,17 +302,16 @@ namespace Shadowsocks.Controller
             {
                 Logging.Debug("filter raw statistics");
                 if (RawStatistics == null) return;
-                if (FilteredStatistics == null)
-                {
-                    FilteredStatistics = new Statistics();
-                }
+                var filteredStatistics = new Statistics();
 
                 foreach (var serverAndRecords in RawStatistics)
                 {
                     var server = serverAndRecords.Key;
                     var filteredRecords = serverAndRecords.Value.FindAll(IsValidRecord);
-                    FilteredStatistics[server] = filteredRecords;
+                    filteredStatistics[server] = filteredRecords;
                 }
+
+                FilteredStatistics = filteredStatistics;
             }
             catch (Exception e)
             {
@@ -315,8 +338,7 @@ namespace Shadowsocks.Controller
             catch (Exception e)
             {
                 Logging.LogUsefulException(e);
-                Console.WriteLine($"failed to load statistics; try to reload {_retryInterval.TotalMinutes} minutes later");
-                _recorder.Change(_retryInterval, RecordingInterval);
+                Console.WriteLine($"failed to load statistics; use runtime statistics, some data may be lost");
             }
         }
 
@@ -328,8 +350,7 @@ namespace Shadowsocks.Controller
 
         public void Dispose()
         {
-            _recorder.Dispose();
-            _speedMonior.Dispose();
+            _perSecondTimer.Dispose();
         }
 
         public void UpdateLatency(Server server, int latency)
@@ -372,6 +393,11 @@ namespace Shadowsocks.Controller
             });
         }
 
+        private class Counter
+        {
+            public int count = 0;
+        }
+
         class UpdateRecordsState
         {
             public int counter;

shadowsocks-csharp/Controller/Strategy/StatisticsStrategy.cs

@@ -28,6 +28,7 @@ namespace Shadowsocks.Controller.Strategy
             var servers = controller.GetCurrentConfiguration().configs;
             var randomIndex = new Random().Next() % servers.Count;
             _currentServer = servers[randomIndex];  //choose a server randomly at first
+            // FIXME: consider Statistics and Config changing asynchrously.
             _timer = new Timer(ReloadStatisticsAndChooseAServer);
         }