Skip to content
This repository has been archived by the owner on Jun 3, 2020. It is now read-only.

Remove PoisonPillMsg #162

Merged
merged 2 commits into from
Jan 23, 2019
Merged

Remove PoisonPillMsg #162

merged 2 commits into from
Jan 23, 2019

Conversation

liamsi
Copy link
Contributor

@liamsi liamsi commented Jan 23, 2019

As per #161 we have other means to shut down gracefully.

Note that that integration tests currently use SIGKILL to shut down the processes they've started. I couldn't find a way to send SIGINT in rust without adding another (dev) dependency. If we want an integration test for the sig_hook, or, let the tests use SIGINT (ctrl+c) instead of SIGKILL, we can do so by using libc I think.

 - use kill instead of wait, integration tests will block anyways until
 proper replies are received
Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

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

Looks great! 👍 Hopefully we'll eventually be able to find a way to get Rust to send a SIGTERM or SIGINT to the child process during testing.

@thanethomson thanethomson merged commit 32b91ed into master Jan 23, 2019
@liamsi liamsi deleted the ismail/remove_poison_pill_msg branch January 23, 2019 15:05
@tarcieri
Copy link
Contributor

Nice

@liamsi
Copy link
Contributor Author

liamsi commented Jan 23, 2019

Hopefully we'll eventually be able to find a way to get Rust to send a SIGTERM or SIGINT to the child process during testing.

@thanethomson, I played around with the libc idea before I removed it again:
https://github.com/tendermint/kms/compare/sig_int?expand=1

Although, this works (all tests pass and won't block forever), we see some errors in the logs. Maybe that's OK though. I've did not include this change because of the errors in the logs and the additional dependency.

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.

3 participants