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: graceful exit #684

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MCozhusheck
Copy link
Collaborator

Description

Exiting app during initialization (especially during connecting to other peers) caused app to run some processes in the background.

Motivation and Context

Fix issue describe above.

How Has This Been Tested?

Run TU and exit during initialization while looking for other peers in the network.

What process can a PR reviewer use to test or verify this change?

Same as above

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

Copy link
Contributor

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Tested locally but this does not solve the problem

@MCozhusheck
Copy link
Collaborator Author

Tested locally but this does not solve the problem

Found and fixed and another commit issue with p2pool locking RWLock which caused dangling p2pool process. It was due p2pool trying to get status from status monitor which was already killed.

@@ -68,7 +68,9 @@ impl<TAdapter: ProcessAdapter> ProcessWatcher<TAdapter> {
loop {
select! {
_ = watch_timer.tick() => {
info!(target: LOG_TARGET, "Checking if {} is running", name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
info!(target: LOG_TARGET, "Checking if {} is running", name);
debug!(target: LOG_TARGET, "Checking if {} is running", name);

if child.ping() {
info!(target: LOG_TARGET, "{} is running", name);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
info!(target: LOG_TARGET, "{} is running", name);
debug!(target: LOG_TARGET, "{} is running", name);

@@ -147,11 +147,12 @@ impl P2poolManager {
.await?;
process_watcher.wait_ready().await?;
if let Some(status_monitor) = &process_watcher.status_monitor {
loop {
for attempt in 1..=5 {
sleep(Duration::from_secs(5)).await;
Copy link
Contributor

Choose a reason for hiding this comment

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

5 seconds is very long here, rather make it 1 second but run more attempts

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