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

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented May 9, 2023

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.

@ethouris ethouris added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels May 9, 2023
@ethouris ethouris added this to the v1.6.0 milestone May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[core] Area: Changes in SRT library core Type: Bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MAINT] Check properly set epoll flags on the listener and accepted socket upon connection
2 participants