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 DeferExitHandler function #848

Merged
merged 4 commits into from Jan 29, 2019
Merged

Conversation

@georgijd
Copy link
Contributor

@georgijd georgijd commented Oct 22, 2018

Useful for running exit handlers in the same order as defer statements

Useful for running exit handlers in the same order as defer statements
@georgijd
Copy link
Contributor Author

@georgijd georgijd commented Jan 6, 2019

Hi,

Is there any chance you guys could have a look at this one? We've been using it for a while now and it's proven very useful.

Thanks

@richpoirier
Copy link
Collaborator

@richpoirier richpoirier commented Jan 7, 2019

@georgijd Can you describe the use case for controlling the order in which the exit handlers run? And why you couldn't just reorder your calls to RegisterExitHandler()?

@georgijd
Copy link
Contributor Author

@georgijd georgijd commented Jan 12, 2019

Hi,

Thank you for your response! A use-case would be if a library which utilises logrus already registers one or more exit handlers. Users of that library might need to run those exit handlers after their own exit handlers which would not be achievable if it's only possible to append exit handlers. I understand that in the majority of cases the exit handlers can be registered in the correct order, but sometimes that's just not possible. Regardless of whether the argument is compelling enough or not, it's always nice to allow people to control the order of such things. Especially when it comes to setup/cleanup procedures and there is little to no cost.

@dgsb
Copy link
Collaborator

@dgsb dgsb commented Jan 16, 2019

@richpoirier this is a common and expected behaviour similar to what is done by atexit. The current implementation is kind of unexpected.

@richpoirier
Copy link
Collaborator

@richpoirier richpoirier commented Jan 18, 2019

Ok. @georgijd could you just update the functions' docs so that it's clear that Register adds the handler to the end of the handlers list and Defer adds it to the beginning? And the unit tests should verify that behavior as well.

Thanks for the PR btw 👍

@georgijd
Copy link
Contributor Author

@georgijd georgijd commented Jan 28, 2019

Hi,

Sorry for the delay. I've been quite busy lately. I've updated the diff with tests and new docstrings

@richpoirier richpoirier merged commit 4ea4861 into sirupsen:master Jan 29, 2019
2 checks passed
@richpoirier
Copy link
Collaborator

@richpoirier richpoirier commented Jan 29, 2019

Thanks @georgijd!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants