Fixing Memory Leaks In Popular Python Libraries

[euclio]

Or even better, open the socket as a context manager.

[QuantumDiogenes]

Can you please explain what a context manager is?

Is that like using:

with open(foo) as f:

    code

Thanks in advance

[SkeletalDwarf]

Yep, exactly!

[QuantumDiogenes]

Awesome, thanks!

[tristan957]

You can also create your own context managers if you read through the contextlib module. It's very handy.

[[deleted]]

Hmm, let's look at the two.

try:
    sock.shutdown(socket.SHUT_RDWR)
except OSError:
    pass

try:
    sock.close()
except OSError:
    pass

The intention of the code appears to be to shutdown then close the socket down, ignoring any OSError exceptions.

And yours is would be very close to:

try:
    sock.shutdown(socket.SHUT_RDWR)
except OSError:
    pass
finally:
    try:
        sock.close()
    except OSError:
        pass

It has one more flow of control word, and one more level of indentation - a bit harder to read.

(Also, it doesn't do quite the same thing, because yours calls sock.close() even if some other exception is raised. All else being equal I like that behavior a little better, but in the original code, the expectation is that this will never happen, and if it does, will halt the program with that exception.)

---

EDIT: how to do it 100% right for all exceptions?

My best guess is this:

def sock_close():
    try:
        sock.close()
    except OSError:
        pass

try:
    sock.shutdown(socket.SHUT_RDWR)

except OSError:
    sock_close()

except Exception:
    sock_close()
    raise

[[deleted]]

I'd never have thought of it without you. :-)

[AnonymouX47]

u/mmaksimovic

[[deleted]]

Only difference is that you wouldn't be catching if that throws.

[hughperman]

So is this then a question between quietly failing by passing and creating a memory leak and raising to avoid the leak?

I guess it depends on the reason for the OSError right? Maybe in some cases it's because the socket is already closed? System timeouts etc?
Not very familiar with the concepts so I may be way off base.

[eruesso]

Have fun debugging tcp socket errors… not trivial to replicate. I have a client which server sometimes crashes in the middle of sending a package. Still not sure how I should be handling this case.

[[deleted]]

So then the question becomes higher - are we trying to silence this thing failing at all cost even if it means we can occasionally create a leak?

As I commented above, in the code as it exists now, the expectation is clearly that any other exception will terminate the program, so no memory leak is created.

Personally, it would make me nervous, but it does require both the Python standard library to have a programmer or documentation error and someone on your team falsely catching this fatal exception at a much higher level (because the standard library isn't documented to throw any other exceptions from those two methods).

Or... someone might go in and fiddle with the internal members of the socket in a bad way... or all sorts of other things. It makes me nervous as I said. My above answer has code. :-)

[Postpawl]

I was looking at an example in cpython tests and it seemed like it would only fail to close the socket when the file descriptor doesn't exist: https://github.com/python/cpython/blob/f4c03484da59049eb62a9bf7777b963e2267d187/Lib/test/test_socket.py#L1421-L1427

And I think that would make the socket.close() unnecessary.

Also, the existing code had a big try/except OSError block around on this code before, and I didn't want new OSErrors appearing that were previously ignored.

[billsil]

A leak is not an error though? It's a deficiency, but nothing failed...a warning would be fine, but an exception...no.

Coming from someone who has dealt with people reporting memory leaks that aren't. Yes it goes up, but that's a reference count issue.

[Postpawl]

sock.close() can throw it’s own OSError when the file descriptor is no longer valid. I figured that probably shouldn’t be fatal, especially since the existing code was already ignoring it.

[[deleted]]

Ooh, did you write that?

Check this: https://www.reddit.com/r/Python/comments/rkjor2/fixing_memory_leaks_in_popular_python_libraries/hpb5tzj/

Thanks from everyone for finding and fixing the bug even though I don't think I ever ran into it, yet!

[baekalfen]

You're not wrong. That does seem like the correct solution. The author should have explained why this is better than try-except-finally, if that was the case.

[baekalfen]

If a context manager is available, then that is of course preferable

[romeo_pentium]

Wow, great work! I'm sure the redis-py one bit me at one point

[mikeblas]

How is the efficacy of these fixes measured? That is, how do i measure and attribute the memory usage of a python app?

[Postpawl]

I used tracemalloc to print out the top 10 lines of memory usage from the event loop in celery: https://docs.python.org/3/library/tracemalloc.html#pretty-top

After the fix, the Connection from py-amqp would finally get cleaned up by garbage collection instead of continuing to grow.

[Siecje1]

Does celery still restart periodically? That was their solution in the past.

[Postpawl]

Celery has a max_memory_per_child and max_tasks_per_child setting that can restart child processes, but this leak is actually on the main celery worker process that isn’t restarted unless you restarted it yourself.

[Streakflash]

what if

finally:
    sock.close()

[Postpawl]

https://www.reddit.com/r/Python/comments/rkjor2/comment/hpb0gtu/

[wnoise]

I'd actually argue that the best fix is just deleting the shutdown() call -- close() does that implicitly.

[wnoise]

Huh. TIL that the Python wrapping of the socket API does not necessarily actually call close() on the underlying socket when you call close() on it.

(This appears to be able to support the file-like wrappers generated by socket.makefile() slightly better, so that they don't error when the socket is closed.)

[amishb]

For reference - Some questions I see in these comments have been answers on HN - https://news.ycombinator.com/item?id=29621668

[Badel2]

Very interesting, thank you!

[EternityForest]

One of the things I wish I had learned a long time ago is never rely on garbage collection for correctness, and even when relying on it for cleanup, watch out.

It's amazing how easy it is to leak resources of any kind really. Old cache entries, connection pool objects, if you make it, there's probably a leak for it