Patchwork [STABLE] run-tests: fix race condition

login
register
mail settings
Submitter Laurent Charignon
Date Jan. 21, 2016, 8:42 p.m.
Message ID <0f975bc6ef3c310d22fb.1453408924@dev5073.prn1.facebook.com>
Download mbox | patch
Permalink /patch/12862/
State Accepted
Headers show

Comments

Laurent Charignon - Jan. 21, 2016, 8:42 p.m.
# HG changeset patch
# User Laurent Charignon <lcharignon@fb.com>
# Date 1453408632 28800
#      Thu Jan 21 12:37:12 2016 -0800
# Branch stable
# Node ID 0f975bc6ef3c310d22fb3016a3204e73bd2fdb1c
# Parent  f62dea3f36975b3daac82e9dcd0ec964cf30c2a7
run-tests: fix race condition

Before this patch, it was possible for run-tests to crash on a race condition.
The race condition happens in the following case:
- the last test finishes and calls: done.put(None)
- the context switches to the main thread that clears the channels list
- the context switches to the last test mentioned above, it tries to access
channels[channel] and crashes
This happened to me while running run-tests.

This patch fixes the issue by clearing the channel before considering that the
test is done.
Bryan O'Sullivan - Jan. 22, 2016, 4:08 a.m.
On Thu, Jan 21, 2016 at 12:42 PM, Laurent Charignon <lcharignon@fb.com>
wrote:

> run-tests: fix race condition
>

Pushed to the clowncopter, thanks.
Bryan O'Sullivan - Jan. 22, 2016, 4:37 a.m.
On Thu, Jan 21, 2016 at 8:08 PM, Bryan O'Sullivan <bos@serpentine.com>
wrote:

> On Thu, Jan 21, 2016 at 12:42 PM, Laurent Charignon <lcharignon@fb.com>
> wrote:
>
>> run-tests: fix race condition
>>
>
> Pushed to the clowncopter, thanks.
>

Actually, upon further consideration, this patch isn't really correct
because what is really needed here is a lock on the use of channels. Your
patch doesn't fix a closely related 100% reproducible exception that I run
into when running many tests at a time.

I sent out a followup patch that fixes both your problem and mine. It's
ready to go to the clowncopter as soon as someone takes a glance; I'm
holding back on the temptation to just push it immediately :-)

Patch

diff --git a/tests/run-tests.py b/tests/run-tests.py
--- a/tests/run-tests.py
+++ b/tests/run-tests.py
@@ -1526,13 +1526,13 @@ 
             channels[channel] = "=" + test.name[5:].split(".")[0]
             try:
                 test(result)
+                channels[channel] = ''
                 done.put(None)
             except KeyboardInterrupt:
-                pass
+                channels[channel] = ''
             except: # re-raises
                 done.put(('!', test, 'run-test raised an error, see traceback'))
                 raise
-            channels[channel] = ''
 
         def stat():
             count = 0