Merge lp:~vila/u1-test-utils/1182875-mail-hangs into lp:u1-test-utils

[Vincent Ladeuil]

Workaround race condition in test_mail.py

This is a stop-gap fix for bug #1182875 which is blocking landings.

First, it declares the thread as a daemonic one so that when the setup fails the process can be stopped. The tearDownClass appears to not be run when an error occurs in setUpClass so the thread is left running.

The second part makes an explicit time.sleep() to work around the lack of explicit synchronization mechanism with localmail.run().

A better fix would probably be to run the mail server in a different process and ensures they give back the port they're running on (addressing the issue encountered here).

[Leo Arias (elopio)]

<elopio> vila: I didn't now about the daemon attribute. Looks good.
<elopio> but the sleep doesn't. Or maybe I misunderstood the docs.
<elopio> vila: my idea with this was that settimeout will fail after 10 seconds of trying to connect.
<vila> elopio: yeah, you know how much I love sleep :-/
<elopio> so the sleep, as I understand, will not help with the problem.
<vila> elopio: except it does ;)
<vila> elopio: but the timeout doesn't
<elopio> vila: could you reproduce it?
<vila> elopio: I did, that's how I found the sleep() call was working around the issue
<cgoldberg> i had a cool retry decorator a while back.. with a timeout... might be of use
<vila> elopio: and I suspect it happens more on jenkins because of the load
<vila> there
<elopio> vila: reading at the documentation of settimeout again, I think we might be missing a call to setblocking.
<elopio> ah, no, blocking mode is the default.
<vila> cgoldberg: that may be helpful ! but in this specific case I would hesitate to retry a setUpClass method
<elopio> then I don't understand. But if the sleep solves your problem, I'll approve it.
<vila> elopio: well, my problem in this case is that landing on u1-t-u are blocked ;)
<vila> elopio: the root issue is that the client is trying to connect before the server has started
<vila> elopio: which is what you wanted to test right ?
<elopio> vila: yes. Yesterday I tried to reproduce the problem, asked on ops, and still didn't understand what was goig on.
<vila> elopio: so I don't think this test is reliable, I've proposed the workaround to unblock landing while we discuss a better fix
<elopio> vila: right. What I don't understand is why settimeout + connect doesn't work. According to the docs it should. So what I thought was that we were waiting 10 seconds, and the server hasn't started.
<vila> elopio: well, sleep(0) was failing, sleep(0.1) passed, so it's far from requiring 10 seconds to start
<elopio> if that's the case, waiting 10.1 seconds would be the same. But, as I said, if you could reproduce the scenario and this works
<elopio> I'll approve now and understand later.

review: Approve

=== modified file 'u1testutils/selftests/unit/test_mail.py'
--- u1testutils/selftests/unit/test_mail.py	2013-01-21 16:19:08 +0000
+++ u1testutils/selftests/unit/test_mail.py	2013-05-22 13:28:51 +0000
@@ -16,6 +16,7 @@
 
 import socket
 import threading
+import time
 import unittest
 
 import localmail
@@ -33,7 +34,17 @@
     def setUpClass(class_):
         class_.localmail_thread = threading.Thread(
             target=localmail.run, args=(class_.SMTP_PORT, settings.IMAP_PORT))
+        # Avoid hanging the main process when something goes wrong
+        class_.localmail_thread.daemon = True
         class_.localmail_thread.start()
+        # There is a race here as there is no (easy, that I know of) way to
+        # guarantee that the server has reached the point that it is listening
+        # to the socket. This leads to conection errors in these cases.  So the
+        # workaround is to... sleep :-/ The proper way to handle this would be
+        # to add an Event() in the server that can be waited on in the client
+        # (or any other sync mechanism including making sure run() blocks until
+        # the listen calls have been really executed) -- vila 2013-05-22
+        time.sleep(0.1)
         # Try the port to make sure the server started.
         socket_ = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
         socket_.settimeout(10)