Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
FalcoGer opened this issue Sep 14, 2018 · 4 comments
Open

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

FalcoGer opened this issue Sep 14, 2018 · 4 comments

Comments

@FalcoGer
Copy link

FalcoGer commented Sep 14, 2018

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
Copy link

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

catch
{
KnxConnectionTunneling.RevertSingleSequenceNumber();
return null;
}

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
SendData(CreateRequestStatusDatagram(destinationAddress));

@FalcoGer
Copy link
Author

FalcoGer commented Oct 2, 2018

I am using tunneling

@SystemKeeper
Copy link

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

@FalcoGer
Copy link
Author

FalcoGer commented Oct 4, 2018

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants