[Merged by Bors] - Fix memory leaks in p2p #2351
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
y0sher
reviewed
Mar 27, 2021
|
Marking as draft for now as I still need to add regression tests |
msgcon does not need its own closer. It's now closed from inside the UDP wrapper. Otherwise, we get a panic on trying to close a closed channel.
Mocks the msgConn in the test. Added a very simple interface to make this easier.
Need to init this channel manually in test
noamnelke
reviewed
Mar 29, 2021
y0sher
reviewed
Mar 29, 2021
lrettig
added a commit
that referenced
this pull request
Mar 30, 2021
Fix typo Spotter: @y0sher Bugfix to pass tests msgcon does not need its own closer. It's now closed from inside the UDP wrapper. Otherwise, we get a panic on trying to close a closed channel. Test that msgConn is closed on UDP conn eviction Mocks the msgConn in the test. Added a very simple interface to make this easier. Another test fix Need to init this channel manually in test Return conn token if incoming conn fails Spotter: @y0sher Formatting Add regression test for net max pending conn Fix merge conflicts and make linter happy
lrettig
added a commit
that referenced
this pull request
Mar 30, 2021
Fix typo Spotter: @y0sher Bugfix to pass tests msgcon does not need its own closer. It's now closed from inside the UDP wrapper. Otherwise, we get a panic on trying to close a closed channel. Test that msgConn is closed on UDP conn eviction Mocks the msgConn in the test. Added a very simple interface to make this easier. Another test fix Need to init this channel manually in test Return conn token if incoming conn fails Spotter: @y0sher Formatting Add regression test for net max pending conn Fix merge conflicts and make linter happy
lrettig
added a commit
that referenced
this pull request
Mar 30, 2021
Fix typo Spotter: @y0sher Bugfix to pass tests msgcon does not need its own closer. It's now closed from inside the UDP wrapper. Otherwise, we get a panic on trying to close a closed channel. Test that msgConn is closed on UDP conn eviction Mocks the msgConn in the test. Added a very simple interface to make this easier. Another test fix Need to init this channel manually in test Return conn token if incoming conn fails Spotter: @y0sher Formatting Add regression test for net max pending conn Fix merge conflicts and make linter happy Fix concurrency issue in test
lrettig
added a commit
that referenced
this pull request
Mar 30, 2021
Fix typo Spotter: @y0sher Bugfix to pass tests msgcon does not need its own closer. It's now closed from inside the UDP wrapper. Otherwise, we get a panic on trying to close a closed channel. Test that msgConn is closed on UDP conn eviction Mocks the msgConn in the test. Added a very simple interface to make this easier. Another test fix Need to init this channel manually in test Return conn token if incoming conn fails Spotter: @y0sher Formatting Add regression test for net max pending conn Fix merge conflicts and make linter happy Fix concurrency issue in test Linter
lrettig
added a commit
that referenced
this pull request
Mar 30, 2021
Fix typo Spotter: @y0sher Bugfix to pass tests msgcon does not need its own closer. It's now closed from inside the UDP wrapper. Otherwise, we get a panic on trying to close a closed channel. Test that msgConn is closed on UDP conn eviction Mocks the msgConn in the test. Added a very simple interface to make this easier. Another test fix Need to init this channel manually in test Return conn token if incoming conn fails Spotter: @y0sher Formatting Add regression test for net max pending conn Fix merge conflicts and make linter happy Fix concurrency issue in test Linter Minor test cleanup Add shutdowns to tests
lrettig
added a commit
that referenced
this pull request
Mar 30, 2021
Fix typo Spotter: @y0sher Bugfix to pass tests msgcon does not need its own closer. It's now closed from inside the UDP wrapper. Otherwise, we get a panic on trying to close a closed channel. Test that msgConn is closed on UDP conn eviction Mocks the msgConn in the test. Added a very simple interface to make this easier. Another test fix Need to init this channel manually in test Return conn token if incoming conn fails Spotter: @y0sher Formatting Add regression test for net max pending conn Fix merge conflicts and make linter happy Fix concurrency issue in test Linter Minor test cleanup Add shutdowns to tests
y0sher
reviewed
Mar 31, 2021
| } | ||
|
|
||
| // The connection is automatically closed when there's no more to read, no need to close it here | ||
| conn.beginEventProcessing() |
There was a problem hiding this comment.
isn't this a blocking function so the defer call above will never be executed until beginEventProcessing terminates which is basically when the connection is closed ?
There was a problem hiding this comment.
yes, it's blocking, that's why this whole function gets run inside a goroutine. Strictly speaking we don't need a defer here but I still think it's cleaner, more idiomatic go: this is a form of cleanup that should happen even if, e.g., there's a panic in conn.beginEventProcessing(). Let me know if you disagree.
There was a problem hiding this comment.
the defer works and idiomatic go because we have 2 return cases. though the actual problem I see is that we're not releasing pending channel until the connection is closed, this was not what this code was intended to do once the connection is running and open it is not pending anymore and we don't need to hold the channel.
There was a problem hiding this comment.
I agree about cleaning up and the panic case though.
There was a problem hiding this comment.
Not sure I understand. The reason this is a memory leak is that we were releasing pending before closing the connection. If we do this, there can be any number of such connections at one time. Look here, you can see there are hundreds of them: #2313 (comment). This caused our managed miners to run out of memory and crash, as far as I can tell. That's why I modified this not to release until the connection closes. The current testnet is running with this code and seems stable!
lrettig
added a commit
that referenced
this pull request
Mar 31, 2021
Fix typo Spotter: @y0sher Bugfix to pass tests msgcon does not need its own closer. It's now closed from inside the UDP wrapper. Otherwise, we get a panic on trying to close a closed channel. Test that msgConn is closed on UDP conn eviction Mocks the msgConn in the test. Added a very simple interface to make this easier. Another test fix Need to init this channel manually in test Return conn token if incoming conn fails Spotter: @y0sher Formatting Add regression test for net max pending conn Fix merge conflicts and make linter happy Fix concurrency issue in test Linter Minor test cleanup Add shutdowns to tests
lrettig
added a commit
that referenced
this pull request
Apr 1, 2021
Fix typo Spotter: @y0sher Bugfix to pass tests msgcon does not need its own closer. It's now closed from inside the UDP wrapper. Otherwise, we get a panic on trying to close a closed channel. Test that msgConn is closed on UDP conn eviction Mocks the msgConn in the test. Added a very simple interface to make this easier. Another test fix Need to init this channel manually in test Return conn token if incoming conn fails Spotter: @y0sher Formatting Add regression test for net max pending conn Fix merge conflicts and make linter happy Fix concurrency issue in test Linter Minor test cleanup Add shutdowns to tests
y0sher
approved these changes
Apr 18, 2021
|
bors merge |
bors bot
pushed a commit
that referenced
this pull request
Apr 21, 2021
## Motivation Closes #2313 ## Changes - Correctly close msgConn in UDP listener when closing UDP connection wrapper - Fix P2P incoming connection handler to correctly handle max conns ## Test Plan Adds regression tests for both fixed memory leaks: - test to make sure that new network connections aren't added beyond MaxPendingConnections, until one is released - test to make sure that msgConnection is closed when a UDP connection is evicted ## TODO <!-- This section should be removed when all items are complete --> - [x] Explain motivation or link existing issue(s) - [x] Test changes and document test plan - [ ] Update documentation as needed ## DevOps Notes <!-- Please uncheck these items as applicable to make DevOps aware of changes that may affect releases --> - [x] This PR does not require configuration changes (e.g., environment variables, GitHub secrets, VM resources) - [x] This PR does not affect public APIs - [x] This PR does not rely on a new version of external services (PoET, elasticsearch, etc.) - [ ] This PR does not make changes to log messages (which monitoring infrastructure may rely on)
|
Pull request successfully merged into develop. Build succeeded: |
bors
bot
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
Closes #2313
Changes
Test Plan
Adds regression tests for both fixed memory leaks:
TODO
DevOps Notes