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

bug: yamux Futures leak #1165

Closed
gabrielmer opened this issue Aug 6, 2024 · 2 comments · Fixed by #1171
Closed

bug: yamux Futures leak #1165

gabrielmer opened this issue Aug 6, 2024 · 2 comments · Fixed by #1171
Assignees

Comments

@gabrielmer
Copy link
Contributor

When running tests on nwaku and activating dumpPendingFutures from nim-chronos, we noticed that the pending futures grow indefinitely in yamux.

Most of them come from the following line

try: # https://github.com/status-im/nim-chronos/issues/516
discard await race(channel.closedRemotely.wait(), channel.receivedData.wait())

For example

Future[4433822480] with name "AsyncEvent.wait" created at <asyncsync.nim:171> and state = Pending

I see these Futures get created at a really fast pace, and not seeing them getting completed. Even if they do eventually get completed, if we create futures at a faster rate than what it takes to complete them, the pending futures would grow indefinitely.

When using mplex instead, the amount of pending Futures is stable and doesn't grow indefinitely

Version: nim-libp2p 94d93cb

Attaching logs with prints of the pending Futures for both yamux and mplex

logs_yamux.txt
logs_mplex.txt

@lchenut
Copy link
Collaborator

lchenut commented Aug 12, 2024

I'll summarize here what we discussed in private about this issue:
Both channel.closedRemotely.wait() and channel.receivedData.wait() are Future created just for this specific line.
race await for one Future to be finished. By design all the others unfinished Futures are not cancelled when the first Future is completed. Therefore, each time readOnce is called (and it's frequent) two Futures are created and only one is finished for sure by the end of the procedure.

We can reproduce the problem with the following code (compiling with -d:chronosFutureTracking)

import chronos

when defined(chronosFutureTracking):
  proc dumpChronosInfo() {.async.} =
    while true:
      echo "\n----------- Dumping Chronos Info -----------"
      echo dumpPendingFutures(OnlyPending)
      await sleepAsync(1.seconds)

  asyncSpawn dumpChronosInfo()

proc main() {.async.} =
  while true:
    discard await race(sleepAsync(50.millis()), sleepAsync(10.seconds()))

waitFor(main())

#1171 will fix it. It's probably not a perfect solution. #1175 could help make it better.

@diegomrsantos
Copy link
Collaborator

@lchenut thank you for the summary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants