New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Race in test_docxmlrpc.py #71801
Comments
The test test_docxmlrpc.py will sometimes hang because of a timing race. I've verified that this code is the same up to version 3.5 and master at https://github.com/python/cpython/blob/master/Lib/test/test_docxmlrpc.py A proposed patch is attached. |
I don’t understand why we have so many tests that assign the server port in the server thread, and then use some sort of synchronization to get it to the client thread. IMO it would be simpler in this case to do something like: def setUp(self):
serv = DOCXMLRPCServer(...)
self.addCleanup(serv.server_close)
[_, PORT] = serv.server_address # Eliminates “ready“ event
# Other server setup here
thread = threading.Thread(target=serv.serve_forever)
thread.start()
self.addCleanup(thread.join) # Instead of self.evt
self.addCleanup(serv.shutdown)
self.client = httplib.HTTPConnection("localhost:%d" % PORT)
self.addCleanup(self.client.close) |
Because the real port number doesn't exist until the server thread starts running and binds the socket using port number 0. |
The whole point of my suggestion was to bind and set the server socket to listing mode before starting the other thread. The socketserver constructor should do this before returning: >>> s = DocXMLRPCServer(("localhost", 0)) # Calls bind() and listen()
>>> s.server_address # Non-zero port has been allocated
('127.0.0.1', 49806)
>>> c = HTTPConnection(*s.server_address)
>>> c.request("OPTIONS", "*") # No connection refused error |
OK, that's a good point. So I don't know the answer to your question. In some cases it may be mostly that the tests are old and written when the tooling was not as good. |
Here is a patch with my idea of how it should work |
It seems the test infrastructure likes all references to thread objects to be deleted, even if they are no longer running. |
I have a doubt. Added a question on Rietveld. |
Earl: Can you give any more details on your original hang or race condition? Was it related to setting PORT, or shutting down the server, or something else? It is not clear from your patch. I have tried adding artificial sleep() calls at various points but that did not uncover anything. I’m sorry, but in my enthusiasm for rewriting the test I didn’t properly understand your original problem :) |
In the original code, the key to the failure I observed is: # wait for port to be assigned
n = 1000
while n > 0 and PORT is None:
time.sleep(0.001)
n -= 1 This gives a fixed deadline for the server thread to create the DocXMLRPCServer instance and provide the corresponding TCP port on which it is listening. If the server thread is late (or if an exception is thrown -- but this case might work out ok), the TCP port will not be available in the variable PORT. In this case, the client thread blunders on, and inadvertently fails because PORT == None. Upon failure, the test case tries to complete by tearing down the test: def tearDown(self):
self.client.close()
self.evt.wait() The test case waits for self.evt to indicate that the server has completed. However, the server thread is running this: while numrequests > 0:
serv.handle_request()
numrequests -= 1 In other words, the test is deadlocked because the server is waiting to process requests (actually it's waiting for a single request) from the client, but the client has already given up and is waiting for the server thread to complete. |
Thanks for the explanation. It seems a bit strange that the server thread was running so slow while the main thread did one thousand polls over at least one second. Perhaps there is a blocking DNS call hidden somewhere in it somewhere? In any case, I am pretty confident my patch should help, so I will commit it unless there are any objections. |
New changeset 397f05044172 by Martin Panter in branch '3.5': New changeset 7136304ecf4c by Martin Panter in branch '2.7': New changeset 17d688dedfca by Martin Panter in branch 'default': |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: