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: prevent messages from taking too long to be sent #902

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from

Conversation

ChaoticTempest
Copy link
Member

messages will occasionally take over 1s to send, which can stall the node from doing work. This PR makes all the send_encrypted be ran concurrently such that the node don't wait until the previous send_encrypted gets completed. I've seen the message sending portion go from 1-2s max to 300ms max so this should be a good improvement with no breaking changes

ailisp
ailisp previously approved these changes Oct 18, 2024
Copy link
Member

@ailisp ailisp left a comment

Choose a reason for hiding this comment

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

Nice improvement by concurrently sending messages!

ppca
ppca previously approved these changes Oct 18, 2024
Copy link
Contributor

@ppca ppca left a comment

Choose a reason for hiding this comment

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

LGTM except the tracing comment.

chain-signatures/node/src/protocol/mod.rs Show resolved Hide resolved
chain-signatures/node/src/http_client.rs Outdated Show resolved Hide resolved
@ChaoticTempest ChaoticTempest dismissed stale reviews from ppca and ailisp via d8d894b October 19, 2024 07:42
volovyks
volovyks previously approved these changes Oct 21, 2024
Copy link
Collaborator

@volovyks volovyks left a comment

Choose a reason for hiding this comment

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

Looks good

chain-signatures/node/src/protocol/mod.rs Show resolved Hide resolved
chain-signatures/node/src/http_client.rs Show resolved Hide resolved
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.

4 participants