From 0a8c9d4eee807b374dda5b78abd28b44ea84b489 Mon Sep 17 00:00:00 2001 From: lonelam Date: Thu, 3 Oct 2019 07:12:43 +0800 Subject: [PATCH] FIX: some crash about thread-safe in statistics --- .../Service/AvailabilityStatistics.cs | 112 +++++++++++------- .../Controller/Strategy/StatisticsStrategy.cs | 1 + 2 files changed, 70 insertions(+), 43 deletions(-) diff --git a/shadowsocks-csharp/Controller/Service/AvailabilityStatistics.cs b/shadowsocks-csharp/Controller/Service/AvailabilityStatistics.cs index 6ce5eb45..7cbeb743 100644 --- a/shadowsocks-csharp/Controller/Service/AvailabilityStatistics.cs +++ b/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()); var outR = _outboundSpeedRecords.GetOrAdd(id, (k) => new List()); @@ -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(); UpdateRecordsState state = new UpdateRecordsState(); - state.counter = _controller.GetCurrentConfiguration().configs.Count; - foreach (var server in _controller.GetCurrentConfiguration().configs) - { - var id = server.Identifier(); - List inboundSpeedRecords = null; - List outboundSpeedRecords = null; - List 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 inboundSpeedRecords = null; + List outboundSpeedRecords = null; + List 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; diff --git a/shadowsocks-csharp/Controller/Strategy/StatisticsStrategy.cs b/shadowsocks-csharp/Controller/Strategy/StatisticsStrategy.cs index 8e15f4bc..73141281 100644 --- a/shadowsocks-csharp/Controller/Strategy/StatisticsStrategy.cs +++ b/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); }