-
Notifications
You must be signed in to change notification settings - Fork 391
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
fix(p2p): Avoid inifinty loop during transport/listener Accept
#3662
Conversation
Signed-off-by: gfanton <[email protected]>
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
Signed-off-by: gfanton <[email protected]>
5e75bbc
to
51b2d7f
Compare
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
…behavior Signed-off-by: gfanton <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sound 💯
Thank you for the fixup 🙏
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
Signed-off-by: gfanton <[email protected]>
This PR aims to fix two potential infinite loops during
Accept
in thep2p
package.First, it removes the
ErrTransportInactive
error. Based on howAccept
is used, I believe the inactive error does not make much sense here.Accept
should always block, even in an inactive state. HavingAccept
in two states—blocking and non-blocking—complicates the logic unnecessarily. Additionally, returning an inactive error creates an infinite loop, as the handler will keep looping overAccept
until a blocking state is reached.If the underlying transport or listener is closed, the context should also be canceled, as this is not a recoverable error. Currently, if the transport or listener is closed, it will loop indefinitely until the context is properly closed. While this situation should rarely occur, since the context should be canceled when closing the transport, I believe it is safe to force the cancellation of the context to avoid any potential deadloop.
EDIT: I've removed the context select from the loop to simplify the logic. The logic remains unchanged, but it avoids having two sources of truth.
This PR also reduces unnecessary error and warning noise when the listener or transport is closed.