Close open resources

[kurtmckee]

There are several places in the test suite and in the production code where resources are opened but not closed. This results in ResourceWarnings while the test suite is running (which Python normally ignores), and is revealed by setting pytest's filterwarning=errors.

This PR introduces the following changes:

• Close sockets that are opened by production code but unclosed when an exception is encountered
• Close resources that are opened by the test suite
• Configure pytest to escalate warnings to errors

If this is merged, it will help ensure that gunicorn consistently closes resources (files and sockets).

[pajod]

Huh. Making warnings fatal keeps CI all green despite the garbage collected file handle in

gunicorn/gunicorn/glogging.py

Line 246 in 79b9a52

config_json = json.load(open(cfg.logconfig_json))

- because we are not testing that in the first place!

Suggested followup: master...pajod:gunicorn:close-resources-followup

---

No objections, though I suspect also a few DeprecationWarnings currently not in CI will pop up - not necessarily a bad thing.

Linking related issues for easier discovery:

• File Descriptors are left open as deleted #2061

[benoitc]

please answer to incline comment. Also revert the change corresponding to the socket. This is not needed.

[benoitc]

shouldn't the garbage collector takes care about it?

[benoitc]

ddon't try to close the cocket there. None should be ok.

[pajod]

shouldn't the garbage collector takes care about it?

Even GC reliably working in our favor is no excuse. We should never make the WSGI application developer figure out which warning is our fault and harmless, and which one is triggered by their code and should be looked into.

[benoitc]

closing sockets like this is unwanted also. The socket is not supposed to be created at this step. So just making it going to None will be enough.

However this is missing a more elaborated error handler to display what's going wrong.

[kurtmckee]

I've answered more fully below; the summary is that relying on the garbage collector without calling .close() will cause Python to throw a ResourceWarning. Simply creating the socket instance is sufficient to cause this problem, as demonstrated in the reproducer code I wrote below.

[benoitc]

why?

[kurtmckee]

I've answered more fully below; the summary is that this ensures the test suite catches ResourceWarnings that might be introduced in the future.

[kurtmckee]

Hello Benoit! Thanks for reviewing this PR. I'm responding here to offer a complete understanding of the changes.

shouldn't the garbage collector takes care about it?

It will, but if the socket's .close() method is not called explicitly then Python will raise a ResourceWarning. ResourceWarning is ignored by Python default, but developers are free to modify how Python handles warnings, which is why this PR configures pytest to escalate warnings to errors and then resolves all of the ResourceWarnings that appeared as a result.

---

don't try to close the socket there. None should be ok.

None is insufficient. Here's a reproducer:

import socket
import warnings

# Configure Python to stop ignoring `ResourceWarning`s.
warnings.filterwarnings("always")


def rely_on_gc():
    # When `s` is reassigned, gc will clean up the socket
    # and a ResourceWarning will be thrown.
    print("rely_on_gc(): start")
    s = socket.socket()
    s = None
    print("rely_on_gc(): end")


def explicit_close():
    print("explicit_close(): start")
    s = socket.socket()
    s.close()
    s = None
    print("explicit_close(): end")


rely_on_gc()
print("---")
explicit_close()

And here's the output:

$ python demo.py 
rely_on_gc(): start
/home/kurt/dev/demo.py:13: ResourceWarning: unclosed <socket.socket fd=3, family=2, type=1, proto=0, laddr=('0.0.0.0', 0)>
  s = None
ResourceWarning: Enable tracemalloc to get the object allocation traceback
rely_on_gc(): end
---
explicit_close(): start
explicit_close(): end

As demonstrated above, setting self.sock to None is insufficient; the socket's .close() method must be called before garbage collection to prevent a ResourceWarning.

---

why [add filterwarnings to the pytest configuration]?

This was actually the first step I took to reveal where ResourceWarnings were getting thrown in the codebase. However, I committed the change only after all ResourceWarnings were resolved.

Adding this configuration to the test suite ensures that future code changes that introduce ResourceWarnings will be caught before they re-enter the gunicorn codebase.

---

revert the change corresponding to the socket. This is not needed

That change is the entire point of this PR -- it guarantees that Python does not throw a ResourceWarning in gunicorn's codebase, and allows developers like me to display all warnings in our deployed environments without seeing a ResourceWarning from within gunicorn.

[kurtmckee]

Hello @benoitc! Please let me know if you have additional questions or feedback. Thanks!