FD leak in SocketServer when request handler throws exception

[luke-jr]

BPO	1429
Nosy	@giampaolo, @vadmium

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2016-02-19.22:48:14.528>
created_at = <Date 2007-11-12.12:27:33.998>
labels = ['type-bug', 'library']
title = 'FD leak in SocketServer when request handler throws exception'
updated_at = <Date 2016-02-19.22:48:14.527>
user = 'https://bugs.python.org/luke-jr'

bugs.python.org fields:

activity = <Date 2016-02-19.22:48:14.527>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2016-02-19.22:48:14.528>
closer = 'martin.panter'
components = ['Library (Lib)']
creation = <Date 2007-11-12.12:27:33.998>
creator = 'luke-jr'
dependencies = []
files = []
hgrepos = []
issue_num = 1429
keywords = []
message_count = 9.0
messages = ['57396', '57471', '104814', '116786', '116788', '130788', '155585', '235599', '260533']
nosy_count = 5.0
nosy_names = ['draghuram', 'giampaolo.rodola', 'luke-jr', 'mcjeff', 'martin.panter']
pr_nums = []
priority = 'high'
resolution = 'works for me'
stage = 'test needed'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue1429'
versions = ['Python 2.7', 'Python 3.2', 'Python 3.3']

[luke-jr]

SocketServer.ThreadingUnixStreamServer leaks file descriptors when a
request handler throws an exception.

[draghuram]

Please provide a small test code that manifests the problem. It is
always the best way to get quick response.

[mcjeff]

I was toying with adding Unix Socket support for one of our internal tools and I thought I ran into a leak in my own code. Searched the bug tracker and found this.

I tried to reproduce, but wasn't able to. Though, if you look at the ThreadingMixIn class, you'll see this:

self.handle_error(request, client_address)
self.close_request(request)

An exception in handle_error, most likely from a subclass, would cause close_request to never fire. Though, the socket.accept'd channel would probably be shut down implicitly when leaving _handle_request_nonblock.

[BreamoreBoy]

This will go nowhere unless a patch is provided that contains code, doc and unit test changes.

[mcjeff]

I'll see if I can get it to reproduce and put a patch together.

[mcjeff]

I entirely forgot I had signed up to look, my apologies.

I'm going through this w/ what's lying on Mercurial's tip, I can't reproduce it at all. I can raise exceptions of various flavors from within the handle method of a StreamRequestHandler and there are no leaking file descriptors.

The only thing worthy of discusion, IMO, is the fact that raising an exception in a handle_error method of a subclass of BaseServer *does* cause the the self.shutdown_request to not run.

Unless I'm mistaken, that does then leave the cleanup of that open socket to GC (but, at that point, anyone overriding handle_error method should know that). Does it make sense to run shutdown_request, even if handle_error throws an Exception?

If anyone thinks that's worthwhile, I can do that.

[mcjeff]

In an effort to walk through bugs in my nosy list, I dug into this and tried to reproduce it to no avail.

Also, as the handle_error method is supposed to handle problems gracefully, calling shutdown on handle_error exception is probably questionable. I'd be happy to submit a patch to do just that if those smarter than I think it is worthwhile, but I don't so much believe it is.

[vadmium]

I think calling shutdown_request(), or at least close_request(), should be done regardless of whether handle_error() fails or not.

[vadmium]

Jeff has tried many times over two years to produce the originally reported bug without success. By code inspection, I cannot see how a request handler exception could cause a leak. Therefore I am closing this as “works for me”.

Working backwards from all the spurious version changes (I wish people wouldn’t do that!), I figure that this was originally opened against Python 2.4 only. It is possible that the bug got fixed in the meantime.

For the question of coping with exceptions from handle_error(), the change proposed in bpo-25139 should cover that.