Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Shutdown Gracefully on Kill or Interrupt. #327

Merged
merged 16 commits into from Oct 1, 2019

Conversation

jsolman
Copy link
Contributor

@jsolman jsolman commented Mar 20, 2019

With this PR Interrupt due to Control+C or process kill will properly be caught and cause the node to be stopped in the same way as typing exit.

@erikzhang
Copy link
Member

In fact, I prefer Control+C to kill the process directly. Sometimes when debugging, you will want to use Control+C when you want to close the process immediately.

@jsolman
Copy link
Contributor Author

jsolman commented Mar 20, 2019

It still generally closes in less than 1 second. It is a best practice to shutdown cleanly.

@vncoelho
Copy link
Member

vncoelho commented Mar 20, 2019

I also like this, Erik...hauajaha
ctrl+c is the power. I never had problem with it.

But I think @jsolman wants to avoid crashes.

@jsolman
Copy link
Contributor Author

jsolman commented Mar 20, 2019

Sometimes when debugging, you will want to use Control+C when you want to close the process immediately.

@erikzhang
You can send a more forceful type of signal if you want to kill it immediately.

@shargon
Copy link
Member

shargon commented Mar 20, 2019

A lot of red message appears before close

image

Maybe we could avoid this to prevent "scares"

@jsolman
Copy link
Contributor Author

jsolman commented Mar 20, 2019

@shargon I was looking for the best way to do it. I haven’t found a trivial way yet since we don’t have a top level actor.

@jsolman
Copy link
Contributor Author

jsolman commented Mar 20, 2019

I’ll see if I can find a way to get rid of them also in this PR after I finish the consensus PR I’m working on now

shargon
shargon previously approved these changes May 28, 2019
@vncoelho
Copy link
Member

vncoelho commented Jun 3, 2019

I think that maybe this patch can come to neo-cli 3.0, however, let's not modify 2.x.

Is there something else to be done here, @jsolman?

@erikzhang, I also prefer ctrl+c to kill direct. However, in the last core developers meeting I think that the majority agreed that we should avoid crashes...aehauhuea

@jsolman
Copy link
Contributor Author

jsolman commented Jun 5, 2019

There is no reason this should not be on both branches IMO.

@vncoelho
Copy link
Member

vncoelho commented Jun 6, 2019

neo-cli 2.x can historically stay with its behavior, this will not really impact in anything critical.

However, it may be a good practice for Neo 3, if the majority agrees.
As I said, I prefer ctrl+c to be quick and exit, but, without crashing anything critical.

@vncoelho vncoelho closed this Jun 6, 2019
@vncoelho vncoelho reopened this Jun 6, 2019
lock9
lock9 previously approved these changes Jul 21, 2019
@shargon
Copy link
Member

shargon commented Jul 22, 2019

conflicts

@shargon shargon requested review from shargon and lock9 July 22, 2019 09:39
@lock9
Copy link
Contributor

lock9 commented Jul 25, 2019

Hi @jsolman, could you solve the conflicts? Do you want us to solve it for you?

@vncoelho vncoelho mentioned this pull request Aug 7, 2019
@shargon shargon mentioned this pull request Aug 7, 2019
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

My review at #428

@shargon
Copy link
Member

shargon commented Aug 7, 2019

@vncoelho does this work well for you in linux?

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

@shargon, with this PR the ctrl +c is working on linux. Looks good

@shargon
Copy link
Member

shargon commented Aug 7, 2019

@vncoelho with my patch is working too?

@jsolman
Copy link
Contributor Author

jsolman commented Aug 8, 2019

Thanks for rebasing.

@vncoelho
Copy link
Member

vncoelho commented Aug 8, 2019

I did not test yet, @shargon. I want to finish the P2P...aheuahueaea

@vncoelho
Copy link
Member

vncoelho commented Aug 8, 2019

@shargon, In fact, ctrl+c has been always working on linux.
Just failing on that other PR that you made and closed...aehauheuaea ajajaja

I still need to test the patch you opened.

But is this an agreement that this change is good?
I got some Akka errors after killing it with ctrl+c. But the same error was obtained when typing exit.
I think it happens when we kill it very soon, before it really initialized.
I need to double test if I also get these errors without this PR!

@vncoelho
Copy link
Member

@shargon, @jsolman and @erikzhang,

This works nice and ok with ctrl+c.

However, if the client is still killed very fast (just after initialization), this error is still obtained:

[ERROR][08/19/2019 19:37:29][Thread 0004][akka://NeoSystem/system/IO-TCP/$a] Monitored actor [[akka://NeoSystem/user/$b#1681348270]] terminated
Cause: Akka.Actor.DeathPactException: Monitored actor [[akka://NeoSystem/user/$b#1681348270]] terminated
   at Akka.Actor.ActorBase.Unhandled(Object message)
   at Akka.Actor.ActorBase.AroundReceive(Receive receive, Object message)
   at Akka.Actor.ActorCell.ReceiveMessage(Object message)
   at Akka.Actor.ActorCell.AutoReceiveMessage(Envelope envelope)
   at Akka.Actor.ActorCell.Invoke(Envelope envelope)

@shargon
Copy link
Member

shargon commented Aug 20, 2019

In my windows works well, but this exceptions is something to fix in other PR, i think

@shargon shargon requested a review from vncoelho August 25, 2019 18:11
@vncoelho
Copy link
Member

@shargon, I prefer to wait for @erikzhang or @superboyiii approval.

@vncoelho
Copy link
Member

@Tommo-L, can you give a look at this?
#469

Copy link
Contributor

@lock9 lock9 left a comment

Choose a reason for hiding this comment

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

It fixes the control+c problem, thanks.
It prints some messages but I think they are ok.
On my windows machine, without this, control-c makes neo-cli unresponsive.

@Tommo-L
Copy link
Contributor

Tommo-L commented Sep 29, 2019

@Tommo-L, can you give a look at this?
#469

I think it also can fix the exit problem. I'll close #469

@lock9 lock9 merged commit aaf104c into master Oct 1, 2019
@lock9 lock9 deleted the GracefulExitOnKillOrInterrupt branch October 1, 2019 21:51
@vncoelho
Copy link
Member

vncoelho commented Oct 17, 2019

image

That is why I did not want to merge it and told to wait for @erikzhang and @superboyiii review's.
I think it should be reverted. It is quite bad as it is. Application is taking tremendous take to be finished when errors are presented, during tests and experimental phase this makes no sense.

@jsolman
Copy link
Contributor Author

jsolman commented Nov 6, 2019

The red messages on Akka are just notifying the actors shutdown. A different top level actor needs to be added to avoid them.

I have run this on Linux a long time and never saw the hang you are describing @vncoelho. What are the steps you are using to reproduce the hang? What hardware are you running on? What flavor of Linux?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants