Skip to content

Conversation

@qoke
Copy link
Contributor

@qoke qoke commented Apr 16, 2025

Multiple goroutines calling AddNode and RemoveNode cause panics when trying to close a channel that was already closed, which can happen when a node changes network transport quickly (eg mobile->wifi) and reconnects whilst also disconnecting.

Added test and fix

@qoke qoke requested review from juanfont and kradalby as code owners April 16, 2025 21:54
@ghost
Copy link

ghost commented Apr 16, 2025

Pull Request Revisions

RevisionDescription
r2
Simplified goroutine closure parameterModified anonymous goroutine in AddNode to remove redundant nodeID parameter, improving function signature simplicity
r1
Added safe channel closing mechanismIntroduced safeCloseChannel method to handle channel closing with panic recovery and added race condition test for node management
✅ AI review completed for r2
Help React with emojis to give feedback on AI-generated reviews:
  • 👍 means the feedback was helpful and actionable
  • 👎 means the feedback was incorrect or unhelpful
💬 Replying to feedback with a comment helps us improve the system. Your input also contributes to shaping future interactions with the AI reviewer.

We'd love to hear from you—reach out anytime at [email protected].

@kradalby
Copy link
Collaborator

Im on holiday, will take a look when I get back.

@kradalby
Copy link
Collaborator

Good catch!

@kradalby kradalby merged commit 56d085b into juanfont:main Apr 23, 2025
270 of 276 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants