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] Fixed group read-ready epoll events. #2766

Merged

Conversation

maxsharabayko
Copy link
Collaborator

@maxsharabayko maxsharabayko commented Jul 18, 2023

There are two bugs found.

  1. After reading a message the group's read-ready state is not updated.
  2. There is no more need (I think) to update read-readiness upon sending an ACK. With the new receiver buffer reading is no longer blocked by the ACK position.

The function was introduced (changed) in v1.5.0 with the new receiver buffer in PR #2218. Maybe v1.5.0 with the old receiver buffer or earlier SRT versions didn't have this read-ready issue. In any case, there might have been other issues with the previous reading function.

TODO

  • CUDTGroup::m_Positions is no longer used. Remove.
  • Remove unused variable group_read_seq.

@maxsharabayko maxsharabayko added Type: Bug Indicates an unexpected problem or unintended behavior [core] Area: Changes in SRT library core labels Jul 18, 2023
@maxsharabayko maxsharabayko added this to the v1.6.0 milestone Jul 18, 2023
srtcore/core.cpp Outdated Show resolved Hide resolved
CUDTSocket* ps = *si;
if (!ps->core().isRcvBufferReady())

if (!ps->core().isRcvBufferReadyNoLock())
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sets a member socket state to false, doesn't it?

srtcore/buffer_rcv.cpp Outdated Show resolved Hide resolved
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.

3 participants