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

delete streams from the streams map when closing #91

Merged
merged 1 commit into from
Jun 2, 2022

Conversation

marten-seemann
Copy link
Contributor

This should fix the incorrect memory accounting that users have been reporting (for example filecoin-project/lotus#8632).

There are two actions (at least) that release memory:

  • closing / resetting a stream
  • closing the connection, which releases the memory for all streams

These can happen concurrently, which is not a problem, as we're holding the streamLock. We're also only releasing memory with the resource manager when the stream is still in our streams map.
The condition that made us release too much memory was the following: First, we'd close the session, which would release the memory of all streams. At the same time, the user reset one of the streams. This call would execute once the session is closed and all memory is released. However, since we didn't delete the stream from the map, we'd effectively release the memory allocated for that stream twice.

@marten-seemann marten-seemann requested a review from vyzo June 2, 2022 12:05
Copy link
Contributor

@vyzo vyzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

@marten-seemann marten-seemann merged commit e3f92bf into master Jun 2, 2022
@BigLep
Copy link

BigLep commented Jun 2, 2022

@marten-seemann : do we not have a unit test because this isn't easy-enough to unit test?

@marten-seemann
Copy link
Contributor Author

@marten-seemann : do we not have a unit test because this isn't easy-enough to unit test?

Correct. You'd have to trigger the race condition where you close the yamux session at exactly the same time you hit Reset on the stream.

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.

3 participants