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

[core][apps] Fixed wrong +W epoll for listener socket in the test apps #1831

Closed
wants to merge 2 commits into from

Conversation

ethouris
Copy link
Collaborator

@ethouris ethouris commented Feb 24, 2021

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.

@ethouris ethouris self-assigned this Feb 24, 2021
@ethouris ethouris added [apps] Area: Test applications related improvements [core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code labels Feb 24, 2021
@ethouris ethouris added this to the v1.4.3 milestone Feb 24, 2021
@maxsharabayko
Copy link
Collaborator

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.

@maxsharabayko
Copy link
Collaborator

maxsharabayko commented Mar 17, 2021

Fixes #1667?

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.

@ethouris
Copy link
Collaborator Author

Right, therefore I submitted a ticket for ffmpeg to fix that: https://trac.ffmpeg.org/ticket/9142

This fix must then remain frozen until ffmpeg fixes this.

@maxsharabayko maxsharabayko modified the milestones: v1.4.3, v1.4.4 Mar 17, 2021
@maxsharabayko maxsharabayko modified the milestones: v1.4.4, v1.5.0 Jun 22, 2021
maxsharabayko added a commit to maxsharabayko/srt that referenced this pull request Apr 4, 2022
Update listener write-ready only after the new connection.
Was changed in Haivision#1650, but must not be done at all (see Haivision#1831).
maxsharabayko added a commit to maxsharabayko/srt that referenced this pull request Apr 4, 2022
Update listener write-ready only after the new connection.
Was changed in Haivision#1650, but must not be done at all (see Haivision#1831).
maxsharabayko added a commit to maxsharabayko/srt that referenced this pull request Apr 12, 2022
Update listener write-ready only after the new connection.
Was changed in Haivision#1650, but must not be done at all (see Haivision#1831).
maxsharabayko added a commit that referenced this pull request Apr 21, 2022
Update listener write-ready only after the new connection.
Was changed in #1650, but must not be done at all (see #1831).
@maxsharabayko maxsharabayko modified the milestones: Major, v1.6.0 May 8, 2023
@ethouris ethouris changed the title [core][apps] Fixed wrong +W epoll for listener socket [core][apps] Fixed wrong +W epoll for listener socket in the test apps May 8, 2023
ethouris pushed a commit that referenced this pull request May 8, 2023
@maxsharabayko maxsharabayko modified the milestones: v1.5.4, v1.6.0 Dec 20, 2023
@ethouris
Copy link
Collaborator Author

Closing - replaced by #2732.

@ethouris ethouris closed this Feb 21, 2024
@ethouris ethouris deleted the dev-fix-wrong-listener-w branch February 21, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[apps] Area: Test applications related improvements [core] Area: Changes in SRT library core Type: Maintenance Work required to maintain or clean up the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants