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

Use {shutdown, Error} when terminating connection processes #238

Merged
merged 2 commits into from May 5, 2022

Conversation

nickva
Copy link
Collaborator

@nickva nickva commented May 2, 2022

Previously they exited normal, which helped avoid error log spam. However, that also meant any linked helper processes would not exit when the main connection process had been terminated.

To automatically clean up any linked processes, and continue avoiding generating error logs, we can use {shutdown, Error} as the exit reason. That error, along with shutdown atom, are special exit reasons which are considered normal for proc_lib processes and will not generate error logs [1].

Another benefit is having more specific exit reasons (send error, recv error, etc.), which may help with debugging.

[1] https://www.erlang.org/docs/24/man/proc_lib.html#description

Previously they exited `normal`, which helped avoid error log spam. However,
that also meant any linked helper processes would not exit when the main
connection process had been terminated.

To automatically clean up any linked processes, and continue avoiding
generating error logs, we can use `{shutdown, Error}` as the exit reason. That
error, along with `shutdown` atom, are special exit reasons which are
considered `normal` for proc_lib processes and will not generate error logs
[1].

Another benefit is having more specific exit reasons (send error, recv error,
etc.), which may help with debugging.

[1] https://www.erlang.org/docs/24/man/proc_lib.html#description
Copy link
Member

@etrepum etrepum left a comment

Choose a reason for hiding this comment

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

This looks like a good idea, it does impact backwards compatibility though since the behavior has changed.

@nickva
Copy link
Collaborator Author

nickva commented May 3, 2022

@etrepum good point, it would introduce an incompatibility. I had tried to see what it would take to upgrade CouchDB apache/couchdb#4013. There were a few places, most just duplicated the exit(normal) idea so switched those to {shtudown, Error} as well. A few cases caught exit:normal those were the ones to worry about. But, overall it wasn't a large diff considering the size of the code.

I thought of perhaps gating it with a config option but options are not always propagated to all the places where exits happen so might lead to a larger refactoring.

@nickva
Copy link
Collaborator Author

nickva commented May 4, 2022

I added a note to the changes log file about the incompatibility to hopefully make it more visible to the users.

@nickva nickva merged commit fce80ef into mochi:main May 5, 2022
@nickva nickva deleted the use-shutdown-for-process-exits branch May 5, 2022 21:58
@nickva
Copy link
Collaborator Author

nickva commented May 5, 2022

I wonder if it would be a reason for a major version bump 3.0, or not large enough of a change? It could be a chance to drop some of the old Erlang VM compatibility, say <18, to avoid checking for rand module, maps, TLS SNI.

@etrepum
Copy link
Member

etrepum commented May 5, 2022

Totally agree that a major version change is the way to go for this release, and it is the perfect opportunity to ditch <18 compat (which is almost 7 years old now)!

@nickva
Copy link
Collaborator Author

nickva commented May 5, 2022

Makes sense. I can give that a try later tonight (if you don't get to it first). It should be a decent cleanup!

nickva added a commit to nickva/mochiweb that referenced this pull request May 6, 2022
After mochi#238 acceptor processes would exit
with {shutdown, Error} on timeouts and other errors which were `normal` before.
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