Bazaar

Merge lp:~vila/bzr/405745-http-hangs into lp:bzr

  1. 405745-http-hangs
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Superseded
Proposed branch: lp:~vila/bzr/405745-http-hangs
Merge into: lp:bzr
Diff against target: 99 lines
2 files modified
NEWS (+3/-0)
bzrlib/tests/http_server.py (+17/-33)
To merge this branch: bzr merge lp:~vila/bzr/405745-http-hangs
Reviewer Review Type Date Requested Status
Andrew Bennetts Approve
Review via email: mp+12984@code.launchpad.net

This proposal has been superseded by a proposal from 2009-10-08.

To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

Thanks to Cris Boylan that keep nagging me on the problem, this patch
addresses the http tests hanging on AIX.

The crux of the issue is that shutdown() can be implemented differently on various systems
and we were trying to handle the socket from two different threads, which is frowned upon on AIX.

This patch stops the server more "manually" but more importantly more cleanly by
connecting to the server (allowing it to wake up from the listen() call and terminate
cleanly).

The server thread can now be joined without trouble reducing the leaking tests from ~300 to ~200
(when running bzr selftest -s bt.test_http).

Note that I didn't add any test here.
I tried to modify test_http.TestHTTPServer.test_server_start_and_stop to check
that no threads were leaked, but that just pointed out that the threading functions
active_count() and enumerate() are very happy to disagree about the number of active threads
even when called one after the other (I can't demonstrate it but I experimented it in ~5-10%
of my test runs).

Other submissions will follow shortly to address more leaks (first http/1.1, then ftp).

Revision history for this message
Andrew Bennetts (spiv) wrote :

Looks good to me. Nice to see large scary comments get removed!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-10-06 20:45:48 +0000
3+++ NEWS 2009-10-07 10:40:26 +0000
4@@ -203,6 +203,9 @@
5 Testing
6 *******
7
8+* HTTP test servers will leak less threads (and sockets) and will not hang on
9+ AIX anymore. (Vincent Ladeuil, #405745)
10+
11 * Passing ``--lsprof-tests -v`` to bzr selftest will cause lsprof output to
12 be output for every test. Note that this is very verbose! (Robert Collins)
13
14
15=== modified file 'bzrlib/tests/http_server.py'
16--- bzrlib/tests/http_server.py 2009-07-08 15:24:31 +0000
17+++ bzrlib/tests/http_server.py 2009-10-07 10:40:26 +0000
18@@ -324,34 +324,17 @@
19 Since the server may be (surely is, even) in a blocking listen, we
20 shutdown its socket before closing it.
21 """
22- # Note that is this executed as part of the implicit tear down in the
23- # main thread while the server runs in its own thread. The clean way
24- # to tear down the server is to instruct him to stop accepting
25- # connections and wait for the current connection(s) to end
26- # naturally. To end the connection naturally, the http transports
27- # should close their socket when they do not need to talk to the
28- # server anymore. This happens naturally during the garbage collection
29- # phase of the test transport objetcs (the server clients), so we
30- # don't have to worry about them. So, for the server, we must tear
31- # down here, from the main thread, when the test have ended. Note
32- # that since the server is in a blocking operation and since python
33- # use select internally, shutting down the socket is reliable and
34- # relatively clean.
35+ # The server is listening for a last connection, let's give it:
36 try:
37- self.socket.shutdown(socket.SHUT_RDWR)
38+ fake_conn = socket.create_connection(self.server_address)
39+ fake_conn.close()
40 except socket.error, e:
41- # WSAENOTCONN (10057) 'Socket is not connected' is harmless on
42- # windows (occurs before the first connection attempt
43- # vila--20071230)
44-
45- # 'Socket is not connected' can also occur on OSX, with a
46- # "regular" ENOTCONN (when something went wrong during test case
47- # setup leading to self.setUp() *not* being called but
48- # self.tearDown() still being called -- vila20081106
49- if not len(e.args) or e.args[0] not in (errno.ENOTCONN, 10057):
50- raise
51- # Let the server properly close the socket
52- self.server_close()
53+ # But ignore connection errors as the point is to unblock the
54+ # server thread, it may happen that it's not blocked or even not
55+ # started (when something went wrong during test case setup
56+ # leading to self.setUp() *not* being called but self.tearDown()
57+ # still being called)
58+ pass
59
60
61 class TestingHTTPServer(SocketServer.TCPServer, TestingHTTPServerMixin):
62@@ -501,6 +484,9 @@
63 pass
64 else:
65 raise
66+ if self._httpd is not None:
67+ # Let the server properly close the listening socket
68+ self._httpd.server_close()
69
70 def _get_remote_url(self, path):
71 path_parts = path.split(os.path.sep)
72@@ -527,10 +513,10 @@
73 """
74 # XXX: TODO: make the server back onto vfs_server rather than local
75 # disk.
76- if not (backing_transport_server is None or \
77- isinstance(backing_transport_server, local.LocalURLServer)):
78+ if not (backing_transport_server is None
79+ or isinstance(backing_transport_server, local.LocalURLServer)):
80 raise AssertionError(
81- "HTTPServer currently assumes local transport, got %s" % \
82+ "HTTPServer currently assumes local transport, got %s" %
83 backing_transport_server)
84 self._home_dir = os.getcwdu()
85 self._local_path_parts = self._home_dir.split(os.path.sep)
86@@ -556,11 +542,9 @@
87
88 def tearDown(self):
89 """See bzrlib.transport.Server.tearDown."""
90+ self._http_running = False
91 self._httpd.tearDown()
92- self._http_running = False
93- # We don't need to 'self._http_thread.join()' here since the thread is
94- # a daemonic one and will be garbage collected anyway. Joining just
95- # slows us down for no added benefit.
96+ self._http_thread.join()
97
98 def get_url(self):
99 """See bzrlib.transport.Server.get_url."""