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

changes system shutdown order: "user" actor first #56

Merged
merged 1 commit into from
Feb 11, 2019

Conversation

StefanBertels
Copy link
Contributor

I had this small issue on shutdownAll: If an exception is thrown in a "user" actor during shutdown then this error caused an error entry on ProcessSystemLog saying "/root/system/errors" is missing -- because the "system" actor already was shut down.

This PR should fix this by shutting down "/root/user" first and everything else (including "/root/system") after that.

@louthy
Copy link
Owner

louthy commented Feb 11, 2019

@StefanBertels I'm going to accept this, but it's really an inefficient way of getting the result you want. We could easily have a bit of code that does:

    shutdown(User);
    shutdown(System);

Rather than forcing a sort on every node.

@louthy louthy merged commit 7128f57 into louthy:master Feb 11, 2019
@StefanBertels
Copy link
Contributor Author

Thanks for merging.

Yes, I decided to use somewhat ugly and potentially inefficient sorting instead of explicit shutdown calls.

But I don't see the issue in your last comment.

I didn't want to break any current or future code and therefore I refactored in a way that always works -- regardless of the number of children of the root actor (and regardless of their name) => robustness.

I don't see a practical performance issue, neither, because there is not "sort on every node". This is ActorSystem.Dispose() which AFAIK means this is just called once (root node). Additionally there is a very limited number of items to sort at all (probably fixed to two in the current implementation). ;-)

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.

None yet

2 participants