[BUG] Removed setting +W for a listener socket after accept ready

[ethouris]

Fixes #1667

There was some kinda completely "out of sense" setting of +W epoll readiness flag for a listener socket at the moment when a new accepted socket was successfully spawned.

This is actually an old UDT library bug, maybe insignificant, but might bear consequences, it's present at the end of this function (in SRT it was renamed to processConnectionRequest):

int CUDT::listen(sockaddr* addr, CPacket& packet)

The standard rules for setting epoll flags other than those for data-read-ready and data-write-ready should follow the manner of TCP and Linux epoll, that is:

• SRT_EPOLL_OUT is set on the connecting socket to declare connection success. This is coincidentally the same with a statement that every newly connected socket is automatically ready to write, and it remains ready as long as there's still free space in the sender buffer.

• SRT_EPOLL_IN is set on the listener socket to declare that this listener socket has spawned an accepted connection socket and it is ready for extraction. After this socket has been extracted by srt_accept call, this readiness is cleared, unless there's still any other socket spawned waiting for extraction.

According to the rules updated after adding the groups feature there was also an SRT_EPOLL_UPDATE event introduced, which is set on the listener at the moment when a new connection within the group has been spawned, but as the group was connected already, it was handled in the background.

In summary, there are specific conditions when the listener socket can be set SRT_EPOLL_IN and SRT_EPOLL_UPDATE events, but there's no reason no API definition to make the listener SRT_EPOLL_OUT readiness flag. It would be also hard to find any similarity to "writing" to the listener socket.

During the check it was also verified that even though this is removed, the accepted socket still gets this write-readiness, even though it isn't properly set anywhere. This is because the first time when you can add the socket to any epoll container is after accepting it, and the procedure of adding a socket to the epoll container involves also subscribing at that socket for any readiness changes. When this is done for the first time of the socket, it also updates its own readiness flags, and this is done at this moment to keep the data updated and the condition as to whether the socket is ready for particular event is verified at this time. In other words, there's no container that keeps the epoll readiness flags of the socket; there's only an epoll container that keeps these flags, but it is foreign to the socket and gets updated only per subscription.

Additionally, it turned out that the testing applications contained a bug in the non-blocking mode by subscribing the listener socket to SRT_EPOLL_OUT, which was occasionally working due to this bug. This was also fixed accordingly.

ADDITIONAL NOTES by @maxsharabayko:

Some notes to save the reviewing context.

Listener socket read-readiness on connection is checked with TestEPoll.SimpleAsync.
Write readiness is not checked. Maybe the test should be modified a bit to add SRT_EPOLL_OUT event type to subscription here, so that we check write-readiness is never triggered.

TODO

• Check if the change proposed would break FFMpeg integration

As noted by @rndi:

SRT listener socket is created with +W and then used in the listening function. https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/libsrt.c#L186
The "write readiness" is added in the create function itself: https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/libsrt.c#L168
Also: https://github.com/FFmpeg/FFmpeg/blob/master/libavformat/libsrt.c#L438.

A ticket submitted to FFMPEG to fix this: https://trac.ffmpeg.org/ticket/9142

The last stance on that is that they are ok with fixing this in SRT.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.88%. Comparing base (09f35c0) to head (ca45739).
Report is 117 commits behind head on develop/v1.6.0.

Additional details and impacted files

@@                Coverage Diff                 @@
##           develop/v1.6.0    #2732      +/-   ##
==================================================
- Coverage           67.20%   66.88%   -0.33%     
==================================================
  Files                 102      102              
  Lines               20269    20266       -3     
==================================================
- Hits                13622    13555      -67     
- Misses               6647     6711      +64     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.