mochi / mochiweb

MochiWeb is an Erlang library for building lightweight HTTP servers.
Other
1.86k stars 474 forks source link

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

Closed nickva closed 2 years ago

nickva commented 2 years ago

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

nickva commented 2 years ago

@etrepum good point, it would introduce an incompatibility. I had tried to see what it would take to upgrade CouchDB https://github.com/apache/couchdb/pull/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 commented 2 years ago

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

nickva commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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