-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: integrating connection change event #14
Conversation
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.
LGTM! Thanks for it! 💯
I just added some nitpick comments that I hope you find useful
@@ -341,6 +341,7 @@ import ( | |||
const requestTimeout = 30 * time.Second | |||
const MsgChanBufferSize = 100 | |||
const TopicHealthChanBufferSize = 100 | |||
const ConnectionChangeChanBufferSize = 100 |
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.
Is it possible to reach the limit? Does it block when that happens or maybe crashes?
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.
Ooh great point!
My understanding is that when we reach the limit, i.e. we fill the channel's buffer without reading anything, subsequent attempts to write in the buffer will be blocked. So basically, calls to the callback will be blocked when we try to push a new value to the channel. But the program should continue as we don't await for the callbacks to finish
@richard-ramos for sure knows better + what are best practices for the use of channels
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.
If the channel is filled, then the callback trying to push to the channel will be blocked, this means that the async task in nwaku will be blocked as well.
It does not pose a problem during the normal execution, as these tasks are async in nwaku so the program will continue. However, i do not know what happens if you call waku_stop and waku_destroy with the callback still blocked.
Seems like an edge case tho (100 connection/disconnection events, and having these events not being consumed).
00bf396
to
7e0fe6c
Compare
Integrating the connection change event and adding a test for it.
This requires the following nwaku PR to be merged: waku-org/nwaku#3225