Device discovery not reliable?!

[calimero-project]

Device discovery not reliable?!

[bmalinowsky]

Hi, for you setup the set timeout does not matter. 2 seconds or so should be plenty time.

Do you know whether there actually is a response received, and whether it was discarded?

[bertvermeiren]

Hi,

I once played with the library and even attempted to write my own java
implementation.
From what I see in the github code base there is a possibility to not see
the reply when doing discovering.

I have a lot of experience in the java world as a professional with multi
threaded behaviour.

Within the Discoverer class you see following code when sending out the
multicast packet for discovery :

s.send(new DatagramPacket(buf, buf.length, SYSTEM_SETUP_MULTICAST,
SEARCH_PORT));
synchronized (receivers) {
final ReceiverLoop l = startReceiver(s, timeout,
nifName + localAddr.getHostAddress());
receivers.add(l);
return l;
}

The time between sending out and starting up your listener thread reading
from the socket, the device can already have sent back the reply and your
reader thread is not started yet.
You should first start your reader thread before sending out the multicast.

Just my experience.

Greetings

Bert Vermeiren.

2016-09-28 11:24 GMT+02:00 bmalinowsky [email protected]:

Hi, for you setup the set timeout does not matter. 2 seconds or so should
be plenty time.

Do you know whether there actually is a response received, and whether it
was discarded?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#36 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALekjev6klfHO826qAdPUST88CSklRXdks5qujI_gaJpZM4KG15T
.

[tuxedo0801]

Thanks Bert,
that indeed looks like a timing/threading-issue... One should definitely start the receiver before sending, otherwise response might receive before the receiver listens.

[tuxedo0801]

I had a deeper look at the code and found more than one section where the receiver listens AFTER the datagram has been sent:

1. https://github.com/calimero-project/calimero-core/blob/master/src/tuwien/auto/calimero/knxnetip/Discoverer.java#L454

2. https://github.com/calimero-project/calimero-core/blob/master/src/tuwien/auto/calimero/knxnetip/Discoverer.java#L517

Beside that, there are a few optimization options:

a) https://github.com/calimero-project/calimero-core/blob/master/src/tuwien/auto/calimero/knxnetip/Discoverer.java#L518
You don't need to synchronize on a synchronized list when adding new items to the collection

b) https://github.com/calimero-project/calimero-core/blob/master/src/tuwien/auto/calimero/knxnetip/Discoverer.java#L385
If I not missed something very important, this section is unnecessary complex:
What sense does it make to convert the list to an array, iterate over the complete array to call quit() on every list-entry and finally remove the complete array from the list? Why not just doe an for-each iteration, call quit() on every line and finally call clear() on the list? Has this to do with the fact that you're not yet using generics?

For 2) the fix is very easy (already changed w.r.t. a) ):

// start receiver BEFORE sending out the datagram
final ReceiverLoop l = startReceiver(s, timeout, nifName + localAddr.getHostAddress());
receivers.add(l);
s.send(new DatagramPacket(buf, buf.length, SYSTEM_SETUP_MULTICAST, SEARCH_PORT));
return l;

I'll try to create a diff/patch and see if my changes will fix the issue.

[tuxedo0801]

I modified the already mentioned section 1) and 2).
Works great. No more detection problems faced so far.

Here's the patch:

diff --git a/src/tuwien/auto/calimero/knxnetip/Discoverer.java b/src/tuwien/auto/calimero/knxnetip/Discoverer.java
index 0b39734..29a3dba 100644
--- a/src/tuwien/auto/calimero/knxnetip/Discoverer.java
+++ b/src/tuwien/auto/calimero/knxnetip/Discoverer.java
@@ -451,14 +451,21 @@
        try {
            final byte[] buf = PacketHelper.toPacket(new DescriptionRequest(nat ? null
                    : (InetSocketAddress) s.getLocalSocketAddress()));
+                        
+                        // start receiver thread
+                        ReceiverLoop receiver = startDescriptionReceiver(s, timeout * 1000, server, s.toString());
            s.send(new DatagramPacket(buf, buf.length, server));
-           final ReceiverLoop looper = new ReceiverLoop(s, 256, timeout * 1000,
-               server);
-           looper.loop();
-           if (looper.thrown != null)
-               throw looper.thrown;
-           if (looper.res != null)
-               return looper.res;
+                        try {
+                            // block until receiver finishs
+                            join(receiver);
+                        } catch (InterruptedException ex) {
+                            // forward the exception to outer try/catch
+                            throw new IOException(ex);
+                        }
+           if (receiver.thrown != null)
+               throw receiver.thrown;
+           if (receiver.res != null)
+               return receiver.res;
        }
        catch (final IOException e) {
            final String msg = "network failure on getting description";
@@ -513,14 +520,12 @@
            final InetSocketAddress res = mcast ? new InetSocketAddress(SYSTEM_SETUP_MULTICAST,
                    s.getLocalPort()) : nat ? null : new InetSocketAddress(localAddr,
                    s.getLocalPort());
-           final byte[] buf = PacketHelper.toPacket(new SearchRequest(res));
-           s.send(new DatagramPacket(buf, buf.length, SYSTEM_SETUP_MULTICAST, SEARCH_PORT));
-           synchronized (receivers) {
-               final ReceiverLoop l = startReceiver(s, timeout,
-                       nifName + localAddr.getHostAddress());
-               receivers.add(l);
-               return l;
-           }
+                        final byte[] buf = PacketHelper.toPacket(new SearchRequest(res));
+                        // start receiver BEFORE sending out the datagram
+                        final ReceiverLoop l = startReceiver(s, timeout, nifName + localAddr.getHostAddress());
+                        receivers.add(l);
+                        s.send(new DatagramPacket(buf, buf.length, SYSTEM_SETUP_MULTICAST, SEARCH_PORT));
+                        return l;
        }
        catch (final IOException e) {
            if (mcast)
@@ -618,6 +623,16 @@
        looper.t.start();
        return looper;
    }
+        
+        private ReceiverLoop startDescriptionReceiver(final DatagramSocket socket, final int timeout, final InetSocketAddress queriedServer,
+       final String name)
+   {
+       final ReceiverLoop looper = new ReceiverLoop(socket, 256, timeout * 1000, queriedServer);
+       looper.t = new Thread(looper, "Discoverer " + name);
+       looper.t.setDaemon(true);
+       looper.t.start();
+       return looper;
+   }

    private final class ReceiverLoop extends UdpSocketLooper implements Runnable
    {

Would be great if you @bmalinowsky could check this and create a 2.3-beta2 or so, to have this fix available before the 2.4 release.

[bertvermeiren]

for b) This code seems correct. For what I can think of - didn't read the
full class although - a new reader can be added (via startSearch) multi
threaded while calling this code.
So you don't want to remove that one from the general list.

If I should design this class, I would make sure you can only use this
discoverer instance once and not multiple times. Makes synchronization much
simpler. (with a close() method for instance)

Regards, Bert.

2016-09-29 8:24 GMT+02:00 Alex [email protected]:

I had a deeper look at the code and found more than one section where the
receiver listens AFTER the datagram has been sent:

1. https://github.com/calimero-project/calimero-core/blob/
   master/src/tuwien/auto/calimero/knxnetip/Discoverer.java#L454

2. https://github.com/calimero-project/calimero-core/blob/
   master/src/tuwien/auto/calimero/knxnetip/Discoverer.java#L517

Beside that, there are a few optimization options:

a) https://github.com/calimero-project/calimero-core/blob/
master/src/tuwien/auto/calimero/knxnetip/Discoverer.java#L518
You don't need to synchronize on a synchronized list when adding new items
to the collection

b) https://github.com/calimero-project/calimero-core/blob/
master/src/tuwien/auto/calimero/knxnetip/Discoverer.java#L385
If I not missed something very important, this section is unnecessary
complex:
What sense does it make to convert the list to an array, iterate over the
complete array to call quit() on every list-entry and finally remove the
complete array from the list? Why not just doe an for-each iteration, call
quit() on every line and finally call clear() on the list? Has this to do
with the fact that you're not yet using generics?

For 2) the fix is very easy (already changed w.r.t. a) ):

// start receiver BEFORE sending out the datagramfinal ReceiverLoop l = startReceiver(s, timeout, nifName + localAddr.getHostAddress());
receivers.add(l);
s.send(new DatagramPacket(buf, buf.length, SYSTEM_SETUP_MULTICAST, SEARCH_PORT));return l;

I'll try to create a diff/patch and see if my changes will fix the issue.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#36 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALekjQsxFCjA9PjbiUPNxAZfS5dPiJ92ks5qu1m3gaJpZM4KG15T
.

[tuxedo0801]

@bmalinowsky
Any comments on the patch? Works like a charm on my side. But I would rather go for an official release instead of an own, patched version ...

[bmalinowsky]

I didn't look into your problem in detail due to current time limitations. Therefore, I also can't exactly state a schedule for v2.3 (which would contain a resolution to this and #37, if applicable).
Since the patch works for you, use it :)

In general (as said, without further insight), I have questions about the underlying hypothesis of Bert, being an issue wrt the order in the execution sequence. I have to take a closer look ...

[bertvermeiren]

Trust me, the network can reply much faster as your java code can run and
starts listening for the response after you send out a UDP datagram packet..
And I'm even not talking here about java garbage collection in between
those 2 operations which definitely can miss the reply.

Regards, Bert.

2016-10-11 14:52 GMT+02:00 bmalinowsky [email protected]:

I didn't look into your problem in detail due to current time limitations.
Therefore, I also can't exactly state a schedule for v2.3 (which would
contain a resolution to this and #37
#37, if
applicable).
Since the patch works for you, use it :)

In general (as said, without further insight), I have questions about the
underlying hypothesis of Bert, being an issue wrt the order in the
execution sequence. I have to take a closer look ...

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#36 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ALekjV2pOP0xZBEP5oUJE8a7poE3Ze9_ks5qy4aUgaJpZM4KG15T
.

[bmalinowsky]

@bertvermeiren

Trust me, the network can reply much faster as your java code can run and
starts listening for the response after you send out a UDP datagram packet..

I even expect it to be faster on certain devices.

By saying there is a race condition, you imply a happens-before requirement on receive -> send (and wrt that, the patch of tuxedo is actually not correct).
One could not write a single-threaded send -> receive; also not run a simple receive loop, because the next call to receive would have to happen before the current one returns (both ofc are possible to do correctly).

Sockets use rx/tx buffers. You can wait a day before invoking receive, the response will be there (usual exceptions apply).

[tuxedo0801]

I still fully agree with Bert. If you use UDP, you should start listenen before you send a request. Otherwise you have a receive-gap where you can miss datagram packets.

The receive method block until a packet is received. It does AFAIK not return already received packets.

wrt my patch: Could you please explain what "is not correct" means?
Without my patch: Sometimes the packet is received slow enough so that the receiver can get it, sometimes it's fast enough so that the receiver don't get it.

Sockets use rx/tx buffers.

Of course.

You can wait a day before invoking receive, the response will be there (usual exceptions apply).

That's true for TCP streams. But with UDP you don't have a stream. You don't even have a "connection".

[bmalinowsky]

Otherwise you have a receive-gap where you can miss datagram packets.

You can always miss datagrams. UDP is an unreliable protocol after all.

It does AFAIK not return already received packets.

It only returns already received "packets".

Could you please explain what "is not correct" means?

Actual thread execution, and invocation of receive (note that my answer was to Bert). I won't go into another discussion here.

That's true for TCP streams. But with UDP you don't have a stream. You don't even have a "connection".

What does TCP and its definition of a connection have to do with that?

[tuxedo0801]

I have to admin: You're right.

I created a short demo, sending udp datagram from A to B with a giant pause and a minimum rx buffer size of 1.. And.... datagram receives.
So maybe I mixed up something...

I'm currently sitting at a different PC, so I cannot re-test with the same environment.
With this PC, I'm not really able to reproduce the issue (2.3-beta, without my patch).

With this PC I had the situation where too many udp packets where received in tcpdump (filter was not explicit enough), so that I was not able to capture a screenshot/copy and paste a situation, where the MDT IP Router answered >5sek after the search request. My discoverer-setup is configured to 5sek.
But I only had this situation once and finally have no proof :-(

Maybe the whole issue depends on a stressed KNX interface?!

I will do some more tests with my other machine where the problem was visible and report back the findings.

[bmalinowsky]

Re-open if there are any conclusive findings...

[tuxedo0801]

Issue still present, but no additional details available.

The provided "fix" in fact solves the issue, reproducible on windows and linux.

Beside participating on this discussion: Did you @bmalinowsky try to run the posted code snippet and thus try to reproduce?