tinc 1.1: missing PONG

[Guus Sliepen]

On Fri, Feb 23, 2018 at 03:04:24PM -0700, Todd C. Miller wrote:

> I've noticed that on Windows during large transfers (such as an
> iperf run), the receiving end sometimes fails to respond to a PING.
> When this happens, the sender closes the connection and both ends
> have to renegotiate.
> 
> My theory is that, due to the properties of the splay tree, the TAP
> device event is almost always at the head of the tree and thus if
> there is traffic, that event will be serviced at the expense of
> meta communication.
> 
> Because splay_search() is used to find the first event returned by
> WSAWaitForMultipleEvents(), the next time through the loop the last
> event serviced will be the first in the events[] array.  This doesn't
> happen with the select() version of the event loop.
> 
> It is possible to eliminate the splay_search() by using an extra
> array to hold io_t pointers but I've had better results just filling
> the events[] array in reverse order.  This results in the most
> recently serviced event being added to the end of events[], which
> should prevent one very busy event from monopolizing the event loop.

The problem is not the order of the events, the problem is that in the
Windows version of the event loop, we only handle one event in each loop
iteration. The select() loop handles all events that have accumulated so
far, so regardless of the order it handles them, it never starves fd. At
least, that was what I thought, until I double checked and found out
that we actually don't in tinc 1.1 (tinc 1.0 is correct though).

So, we have to find a proper fix for both the POSIX and Windows variants
of the event loop in 1.1. For POSIX, the issue is that we might
invalidate the loop iterator due to a callback. It should be easy to
detect that and only exit the loop early if that's really the case. I'll
do that.

For Windows, there are two issues that have to be fixed. First is
that we only handle a single writable IO event. The second is that
WSAWaitForMultipleEvents() only returns one index into the array of
events, even though multiple events might have triggered. How hard would
it be to check for all triggered events?

> With this change I can no longer reproduce the problem of nodes not
> responding to pings.

This will merely move the problem to some other filedescriptor.

> Another solution would to randomly shuffle events[] but that seems
> needlessly complicated.

Yes, that's a bad solution :)

-- 
Met vriendelijke groet / with kind regards,
     Guus Sliepen <guus at tinc-vpn.org>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://www.tinc-vpn.org/pipermail/tinc-devel/attachments/20180226/6b6bc3d8/attachment.sig>