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

Fix: Limit ZMQ Buffer Size for Outgoing Messages #4051

Merged
merged 2 commits into from
Sep 28, 2023
Merged

Conversation

msgmaxim
Copy link
Contributor

@msgmaxim msgmaxim commented Sep 27, 2023

Pull Request

Closes: PRO-858

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

Updated the ZMQ_SNDHWM and ZMQ_IMMEDIATE as discussed. I left ZMQ_RCVHWM unchanged since there is no risk for messages to get stuck in the receive buffer (we only have one "receive" socket shared between all peers, so it is not possible to miss any incoming messages).

This doesn't eliminate the memory leak entirely of course, since we can have bad peers that never come online to consume their messages (I wish we could set a limit on how long messages can stay in the buffer, but I didn't see an option for that) and we don't really have a mechanism to remove their sockets atm. I wonder if we should be detecting such peers and resetting the sockets manually. I might create a linear issue for something like that.

@linear
Copy link

linear bot commented Sep 27, 2023

PRO-858 Set ZMQ_RCVHWM, ZMQ_SNDHWM, ZMQ_IMMEDIATE to resolve zmq "memory leak"

See PRO-357 for details.

These ZMQ options can be used to resolve this problem it seems.
To me the right thing would be to decrease both ZMQ_SNDHWM and ZMQ_RCVHWM to 10 and possibly set ZMQ_IMMEDIATE = true.

@msgmaxim msgmaxim enabled auto-merge (squash) September 28, 2023 01:19
@codecov
Copy link

codecov bot commented Sep 28, 2023

Codecov Report

Merging #4051 (966f18e) into main (d69ebb6) will decrease coverage by 0%.
Report is 4 commits behind head on main.
The diff coverage is 100%.

@@          Coverage Diff           @@
##            main   #4051    +/-   ##
======================================
- Coverage     72%     72%    -0%     
======================================
  Files        369     374     +5     
  Lines      59322   59219   -103     
  Branches   59322   59219   -103     
======================================
- Hits       42676   42509   -167     
- Misses     14528   14603    +75     
+ Partials    2118    2107    -11     
Files Coverage Δ
engine/src/p2p/core/socket.rs 94% <100%> (+1%) ⬆️
engine/src/p2p/core/tests.rs 100% <100%> (ø)

... and 49 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msgmaxim msgmaxim merged commit 406f481 into main Sep 28, 2023
44 checks passed
@msgmaxim msgmaxim deleted the fix/p2p-leak branch September 28, 2023 01:49
dandanlen pushed a commit that referenced this pull request Sep 28, 2023
* fix: limit zmq message buffer size

* chore: fix test by waiting for connection
dandanlen pushed a commit that referenced this pull request Sep 29, 2023
* fix: limit zmq message buffer size

* chore: fix test by waiting for connection
dandanlen pushed a commit that referenced this pull request Oct 4, 2023
* fix: limit zmq message buffer size

* chore: fix test by waiting for connection
dandanlen pushed a commit that referenced this pull request Oct 9, 2023
* fix: limit zmq message buffer size

* chore: fix test by waiting for connection
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.

2 participants