-
Notifications
You must be signed in to change notification settings - Fork 47
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(libp2p): Updating nim-libp2p to fix the wss
connectivity issue
#1848
fix(libp2p): Updating nim-libp2p to fix the wss
connectivity issue
#1848
Conversation
* The `nim-libp2p` adds many changes although we are actually interested in the latest commit: > wstransport.nim: avoid re-raising 'TransportOsError' to avoid stopping `switch.accept` (#929) * The `nim-stew` bump is needed so that the `nim-libp2p` can compile. * The changes in `waku_node.nim` are needed due to the changes in `nim-stew`.
Please don't check it just yet. I'll add reviewers once I confirm CI passes |
fa05e4d
to
aa9e56e
Compare
…Handler' This is aimed to avoid the next exception happening in the wakunode: ``` Unhandled defect: Async procedure (service.nim(74) callHandler) yielded `nil`, are you await'ing a `nil` Future? [AssertionDefect] ```
Thanks so much indeed @rymnc for shedding a lot of light on a bug that I had! I'm super happy about it! |
NICE! that bug was very cryptic! |
I don't understand the fix. Do you have to yield back control to the async runtime because it block something else? |
waku/v2/node/waku_node.nim
Outdated
proc statusAndConfidenceHandler(networkReachability: NetworkReachability, | ||
confidence: Opt[float]): | ||
Future[void] {.gcsafe, raises: [].} = | ||
if confidence.isSome(): | ||
info "Peer reachability status", networkReachability=networkReachability, confidence=confidence.get() | ||
|
||
let retFut = newFuture[void]() | ||
retFut.complete() | ||
return retFut | ||
|
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.
proc statusAndConfidenceHandler(networkReachability: NetworkReachability, | |
confidence: Opt[float]): | |
Future[void] {.gcsafe, raises: [].} = | |
if confidence.isSome(): | |
info "Peer reachability status", networkReachability=networkReachability, confidence=confidence.get() | |
let retFut = newFuture[void]() | |
retFut.complete() | |
return retFut | |
proc statusAndConfidenceHandler(networkReachability: NetworkReachability, | |
confidence: Opt[float]): | |
Future[void] {.gcsafe, async.} = | |
if confidence.isSome(): | |
info "Peer reachability status", networkReachability=networkReachability, confidence=confidence.get() |
Sorry if I wasn't clear on discord, that is what I meant when I said "Some of the failures are because you removed the async from statusAndConfidenceHandler"
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.
Thanks for the comment @Menduist ! It is much better that way!
b869d18
to
93281f9
Compare
…turns a Future[void]
I got confused by
I get it now! |
Description
nim-libp2p
adds many changes although we are actually interested in the latest commit:The
nim-stew
bump is needed so that thenim-libp2p
can compile.The changes in
waku_node.nim
are needed due to the changes innim-stew
.Issue
closes #1831