From 46d95e7411cbb2177e40fc585cd01de44a83969d Mon Sep 17 00:00:00 2001 From: Syrone Wong Date: Thu, 29 Sep 2016 11:00:33 +0800 Subject: [PATCH] Get rid of lock(this) - Use readonly object as lock Info from stackoverflow: If I want to be sure that it will be locked for all threads inside my application: The lock object has to be static, if it locks access to static state. Otherwise it has to be instance, because there's no need to lock state of one class instance, and prevent other threads to work with another class instance at the same time. everyone says that the object has to be "readonly" I didn't found the reason Well, it doesn't have to be. This is just a best practice, which helps you to avoid errors. Signed-off-by: Syrone Wong --- shadowsocks-csharp/Controller/Service/PortForwarder.cs | 5 ++++- shadowsocks-csharp/Controller/Service/TCPRelay.cs | 8 +++++--- shadowsocks-csharp/Encryption/MbedTLSEncryptor.cs | 5 ++++- shadowsocks-csharp/View/LogForm.cs | 7 +++++-- 4 files changed, 18 insertions(+), 7 deletions(-) diff --git a/shadowsocks-csharp/Controller/Service/PortForwarder.cs b/shadowsocks-csharp/Controller/Service/PortForwarder.cs index 5245bb7e..5429b053 100644 --- a/shadowsocks-csharp/Controller/Service/PortForwarder.cs +++ b/shadowsocks-csharp/Controller/Service/PortForwarder.cs @@ -39,6 +39,9 @@ namespace Shadowsocks.Controller // connection receive buffer private byte[] connetionRecvBuffer = new byte[RecvSize]; + // instance-based lock + private readonly object _Lock = new object(); + public void Start(byte[] firstPacket, int length, Socket socket, int targetPort) { this._firstPacket = firstPacket; @@ -220,7 +223,7 @@ namespace Shadowsocks.Controller public void Close() { - lock (this) + lock (_Lock) { if (_closed) { diff --git a/shadowsocks-csharp/Controller/Service/TCPRelay.cs b/shadowsocks-csharp/Controller/Service/TCPRelay.cs index 3ad3f4a3..c9e1ed25 100644 --- a/shadowsocks-csharp/Controller/Service/TCPRelay.cs +++ b/shadowsocks-csharp/Controller/Service/TCPRelay.cs @@ -160,8 +160,10 @@ namespace Shadowsocks.Controller private bool _remoteShutdown = false; private bool _closed = false; - private object _encryptionLock = new object(); - private object _decryptionLock = new object(); + // instance-based lock without static + private readonly object _encryptionLock = new object(); + private readonly object _decryptionLock = new object(); + private readonly object _closeConnLock = new object(); private DateTime _startConnectTime; private DateTime _startReceivingTime; @@ -207,7 +209,7 @@ namespace Shadowsocks.Controller public void Close() { - lock (this) + lock (_closeConnLock) { if (_closed) return; _closed = true; diff --git a/shadowsocks-csharp/Encryption/MbedTLSEncryptor.cs b/shadowsocks-csharp/Encryption/MbedTLSEncryptor.cs index 4de20e70..5d958a1a 100644 --- a/shadowsocks-csharp/Encryption/MbedTLSEncryptor.cs +++ b/shadowsocks-csharp/Encryption/MbedTLSEncryptor.cs @@ -15,6 +15,9 @@ namespace Shadowsocks.Encryption private IntPtr _encryptCtx = IntPtr.Zero; private IntPtr _decryptCtx = IntPtr.Zero; + // instance based lock + private readonly object _Lock = new object(); + public MbedTLSEncryptor(string method, string password, bool onetimeauth, bool isudp) : base(method, password, onetimeauth, isudp) { @@ -120,7 +123,7 @@ namespace Shadowsocks.Encryption protected virtual void Dispose(bool disposing) { - lock (this) + lock (_Lock) { if (_disposed) { diff --git a/shadowsocks-csharp/View/LogForm.cs b/shadowsocks-csharp/View/LogForm.cs index c29ff82d..f910a41a 100644 --- a/shadowsocks-csharp/View/LogForm.cs +++ b/shadowsocks-csharp/View/LogForm.cs @@ -34,6 +34,9 @@ namespace Shadowsocks.View const int BACK_OFFSET = 65536; ShadowsocksController controller; + // global traffic update lock, make it static + private static readonly object _lock = new object(); + #region chart long lastMaxSpeed; ShadowsocksController.QueueLast traffic = new ShadowsocksController.QueueLast(); @@ -71,7 +74,7 @@ namespace Shadowsocks.View long maxSpeed = 0; long lastInbound, lastOutbound; - lock (this) + lock (_lock) { if (traffic.Count == 0) return; @@ -120,7 +123,7 @@ namespace Shadowsocks.View private void controller_TrafficChanged(object sender, EventArgs e) { - lock (this) + lock (_lock) { traffic = new ShadowsocksController.QueueLast(); foreach (var trafficPerSecond in controller.traffic)