Skip to content

Add a testcase for pthreads race conditions #12258

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

Closed
wants to merge 4 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 75 additions & 0 deletions tests/pthread/test_pthread_proxy_hammer.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
#include <errno.h>
#include <stdlib.h>
#include <fcntl.h>
#include <unistd.h>
#include <emscripten.h>
#include <stdio.h>

class random_device
{
int __f_;
public:
// constructors
explicit random_device();
~random_device();

// generating functions
unsigned operator()();
};

random_device::random_device()
{
__f_ = open("/dev/urandom", O_RDONLY);
if (__f_ < 0) abort();
}

random_device::~random_device()
{
close(__f_);
}

unsigned
random_device::operator()()
{
unsigned r;
size_t n = sizeof(r);
char* p = reinterpret_cast<char*>(&r);
while (n > 0)
{
ssize_t s = read(__f_, p, 1);
if (s == 0) abort();
if (s == -1)
{
if (errno != EINTR) abort();
continue;
}
n -= static_cast<size_t>(s);
p += static_cast<size_t>(s);
}
return r;
}

int main() {
int total = 0;
for (int i = 0; i < 1024; i++) {
// printf causes proxying
printf("%d %d\n", i, total);
for (int j = 0; j < 1024; j++) {
// allocation uses a mutex
auto* rd = new random_device();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this be doing directly with the pthread_mutex APIs rather than indirectly depending on the implementation of "/dev/urandom"?

Of you could write the test directly against pthread.h we could also see if it occurs in musl's native configuration.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, the use of /dev/random is not just for a mutex - it's also for proxying (all file I/O is proxied to the main thread). That involves more than just a mutex.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly, I was hoping for something a little more precise .... there is so much going on here its hard to know what this is testing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, definitely... yeah, this is not a great testcase. But it's the smallest I've managed so far that shows the issue, which is really hard to reproduce (as shown by it existing since forever, apparently).

If I have time I can try to reduce this more. But it may be better to focus on figuring out the actual cause of the problem, as that may suggest a testcase. We don't need to merge this urgently and may never merge it I guess.

// reading data causes proxying
for (int k = 0; k < 4; k++) {
total += (*rd)();
}
// make sure the optimizer doesn't remove the allocation
EM_ASM({ out("iter") }, rd);
delete rd;
}
}
printf("done: %d", total);
#ifdef REPORT_RESULT
REPORT_RESULT(0);
#endif
return 0;
}

20 changes: 10 additions & 10 deletions tests/runner.py
Original file line number Diff line number Diff line change
@@ -1364,12 +1364,12 @@ def assert_out_queue_empty(self, who):
self.harness_out_queue.get()
raise Exception('excessive responses from %s' % who)

# @param tries_left: how many more times to try this test, if it fails. browser tests have
# many more causes of flakiness (in particular, they do not run
# synchronously, so we have a timeout, which can be hit if the VM
# we run on stalls temporarily), so we let each test try more than
# once by default
def run_browser(self, html_file, message, expectedResult=None, timeout=None, tries_left=1):
# @param extra_tries: how many more times to try this test, if it fails. browser tests have
# many more causes of flakiness (in particular, they do not run
# synchronously, so we have a timeout, which can be hit if the VM
# we run on stalls temporarily), so we let each test try more than
# once by default
def run_browser(self, html_file, message, expectedResult=None, timeout=None, extra_tries=1):
if not has_browser():
return
if BrowserCore.unresponsive_tests >= BrowserCore.MAX_UNRESPONSIVE_TESTS:
@@ -1408,10 +1408,10 @@ def run_browser(self, html_file, message, expectedResult=None, timeout=None, tri
try:
self.assertIdenticalUrlEncoded(expectedResult, output)
except Exception as e:
if tries_left > 0:
if extra_tries > 0:
print('[test error (see below), automatically retrying]')
print(e)
return self.run_browser(html_file, message, expectedResult, timeout, tries_left - 1)
return self.run_browser(html_file, message, expectedResult, timeout, extra_tries - 1)
else:
raise e
finally:
@@ -1549,7 +1549,7 @@ def btest(self, filename, expected=None, reference=None, force_c=False,
reference_slack=0, manual_reference=False, post_build=None,
args=[], outfile='test.html', message='.', also_proxied=False,
url_suffix='', timeout=None, also_asmjs=False,
manually_trigger_reftest=False):
manually_trigger_reftest=False, extra_tries=1):
assert expected or reference, 'a btest must either expect an output, or have a reference image'
# if we are provided the source and not a path, use that
filename_is_src = '\n' in filename
@@ -1583,7 +1583,7 @@ def btest(self, filename, expected=None, reference=None, force_c=False,
post_build()
if not isinstance(expected, list):
expected = [expected]
self.run_browser(outfile + url_suffix, message, ['/report_result?' + e for e in expected], timeout=timeout)
self.run_browser(outfile + url_suffix, message, ['/report_result?' + e for e in expected], timeout=timeout, extra_tries=extra_tries)

# Tests can opt into being run under asmjs as well
if 'WASM=0' not in args and (also_asmjs or self.also_asmjs):
15 changes: 15 additions & 0 deletions tests/test_browser.py
Original file line number Diff line number Diff line change
@@ -4911,3 +4911,18 @@ def test_zzz_zzz_4GB_fail(self):
# 4GB.
self.emcc_args += ['-O2', '-s', 'ALLOW_MEMORY_GROWTH', '-s', 'MAXIMUM_MEMORY=4GB', '-s', 'ABORTING_MALLOC=0']
self.do_run_in_out_file_test('tests', 'browser', 'test_4GB_fail.cpp', js_engines=[V8_ENGINE])

@unittest.skip("only run this manually, to test for race conditions")
@requires_threads
def test_manual_pthread_proxy_hammer(self):
# the specific symptom of the hangs that were fixed is that the test hangs
# at some point, using 0% CPU. often that occured in 0-200 iterations out
# of the 1024 the test runs by default, but this can vary by machine and
# browser...
self.btest(path_from_root('tests', 'pthread', 'test_pthread_proxy_hammer.cpp'),
expected='0',
args=['-s', 'USE_PTHREADS=1', '-O2', '-s', 'PROXY_TO_PTHREAD'],
timeout=10000,
# don't run this with the default extra_tries value, as this is
# *meant* to notice something random, a race condition.
extra_tries=0)