lifeemotions / knx.net

KNX.net provides a KNX API for .NET
MIT License
101 stars 47 forks source link

RequestStatus and action fails don't clean up lock #44

Open FalcoGer opened 5 years ago

FalcoGer commented 5 years ago

When requesting the status of an invalid group address, the CreateRequestStatusDatagram(destinationAddress) function call in KnxSender.cs:RequestStatus(string) will return null. SendData will be called with null, causing the backend to fail with an exception. At this point when you caught the exception in the main program the next knx action will be encountering the lock that was previously set, causing the program to hang indefinitely

Test case:

// TunnelingConnection conn opened somewhere here
{
  try
  {
    // causes exception: argumentnullexception, because udpSend was trying to send byte array with null
    // should be argumentexception, "groupaddress invalid" or something.
    conn.RequestStatus("12/50/255"); // exception, middle group valid in [0..7]
  }
  catch (Exception ex)
  {
    Console.WriteLine(ex.ToString());
  }
  try
  {
    // this now locks up
    conn.RequestStatus("12/5/255"); // is valid
  }
  catch (Exception ex)
  {
    Console.WriteLine(ex.ToString());
  }
}

And while we are at it, a nice, public check knx addr function that returns true if the address is valid and false otherwise would be a really nice addition.

SystemKeeper commented 5 years ago

Are you using Tunneling? I was only partly able to reproduce your issue with Tunneling (works for Routing): I also get the wrong exception (I get a NullReferenceException) when requesting a status for "12/50/255" but I don't see a lock when requesting "12/5/255" afterwards. I think the problem is here https://github.com/lifeemotions/knx.net/blob/6b50cefa6fe24794efe1a86b089139babcb8c509/src/KNXLib/KnxSenderTunneling.cs#L118-L123 The InvalidKnxAddressException is catched here and null is returned which then is tried to be send via udp. I think we can just replace return null; with throw;. Otherwise we'd need to introduce a null check at https://github.com/lifeemotions/knx.net/blob/6b50cefa6fe24794efe1a86b089139babcb8c509/src/KNXLib/KnxSender.cs#L21

FalcoGer commented 5 years ago

I am using tunneling

SystemKeeper commented 5 years ago

Would you be able to test if the fix I mentioned in my previous comment works for you?

FalcoGer commented 5 years ago

I changed the return null to throw in KnxSenderTunneling.CreateRequestStatusDatagram and KnxSenderTunneling.CreateActionDatagram

during the finally in KnxLockManager.PerformLockedOperation it does call SendUnlockPauseThread

During debugging it froze at the next _sendLock.Wait call, however I believe it is because at that point the connection dropped. running without step by step debugging it does seem to work fine.