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

Add a timeout to the PeerServer event loop. #65

Merged
merged 4 commits into from
Oct 15, 2019
Merged

Conversation

hdevalence
Copy link
Contributor

I think this code could be cleaned up significantly (e.g., removing the
other use of select!) but that's potentially a larger change than this
PR.

@hdevalence hdevalence marked this pull request as ready for review October 11, 2019 01:07
@hdevalence
Copy link
Contributor Author

This closes #51 but the main event loop for the PeerServer is quite messy; I think it could be cleaned up significantly (removing the other select!, inlining some match patterns, moving the logic for each state-handling into separate functions instead of having a 100-line loop body, etc), but this could potentially be done in a separate step / issue?

@hdevalence hdevalence mentioned this pull request Oct 11, 2019
@hdevalence hdevalence requested a review from a team October 11, 2019 01:18
@hdevalence hdevalence force-pushed the peer-request-timeout branch from c7ec58a to 592b1e8 Compare October 11, 2019 16:00
@hdevalence
Copy link
Contributor Author

Going to try cleaning up the event loop a bit.

@hdevalence hdevalence force-pushed the peer-request-timeout branch from e34c818 to acb8848 Compare October 11, 2019 17:18
@hdevalence hdevalence requested review from a team and removed request for a team October 11, 2019 17:26
@hdevalence
Copy link
Contributor Author

Cleaned the event loop, removed the only other use of the select! macro.

I think this code could be cleaned up significantly (e.g., removing the
other use of select!) but that's potentially a larger change than this
PR.
@hdevalence hdevalence force-pushed the peer-request-timeout branch from 00f6c73 to e7f87d4 Compare October 11, 2019 23:32
@hdevalence
Copy link
Contributor Author

Rebased on top of main to resolve conflicts with #66

zebra-network/src/peer/server.rs Show resolved Hide resolved
Ok(new_state) => self.state = new_state,
Ok(new_state) => {
self.state = new_state;
self.request_timer = Some(delay_for(constants::REQUEST_TIMEOUT));
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

None => return,
}
}
match future::select(shutdown_rx.next(), worker_rx.next()).await {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@hdevalence
Copy link
Contributor Author

Those renamings were incomplete, so I'm going to edit them locally, check they wrok, and update the branch

@hdevalence hdevalence force-pushed the peer-request-timeout branch from 956fbdf to 350d292 Compare October 15, 2019 17:49
@hdevalence
Copy link
Contributor Author

amended & updated!

Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

LGTM ✨

@dconnolly dconnolly merged commit 199038e into main Oct 15, 2019
@dconnolly dconnolly deleted the peer-request-timeout branch October 15, 2019 18:49
@mpguerra mpguerra mentioned this pull request Apr 11, 2023
4 tasks
skyl added a commit to skyl/zebra that referenced this pull request Sep 25, 2024
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