CodeBardian / tplink-smartdevices-netstandard

.NET Standard 2.0 Library for Discovering and Operating TP-Link Smart Devices
https://codebardian.github.io/tplink-smartdevices-netstandard/
Apache License 2.0
18 stars 5 forks source link

Avoid sync over async and async over sync calls. #14

Closed renestein closed 3 years ago

renestein commented 3 years ago

For example:

//TPLinkSmartMeterPlug
        public TPLinkSmartMeterPlug(string hostname) : base(hostname)
        {
            Task.Run(async() =>
            {
                await Refresh();   
            }).GetAwaiter().GetResult();
        }

        public new async Task Refresh()
        {
            CurrentPowerUsage = new PowerData(await Execute("emeter", "get_realtime"), HardwareVersion);
            _gainData = await Execute("emeter", "get_vgain_igain");
            await base.Refresh();
        }

//SmartHomeProtocolMessage

        internal async Task<dynamic> Execute(string hostname, int port)
        {
            var messageToSend = SmartHomeProtocolEncoder.Encrypt(JSON);

            var client = new TcpClient();
            client.ConnectAsync(hostname, port).Wait(2000);

            if (!client.Connected)
            {
                return null;
            }

The constructor is synchronous, calls Task.Run (offloading) and then Wait for results. SmartHomeProtocolMessage.Execute is an async method, internally the method calls asynchronous method TcpClient.ConnectAsync, then immediately block (Wait(200)). The main problem is: 1) ConnectAsync may timeout because the ConnectAsync task does not run/does not complete even if the device is ready - Task is not scheduled. When there are more async calls in progress and thread from ThreadPools are already blocked, ThreadPoool detects starvation after a while (15-60s?) and injects a new thread. But the new thread must be created, scheduled, and run - again, this takes time and ConnectAsync timeout is 2 seconds only.

2) Execute may return null even in case the device is available.

(Putting aside the fact that the Connected property on TcpClient class is not very reliable - may be the better strategy is the detection of the failed Write/Send operation?)