unix,win: fix threadpool race condition

[addaleax]

90891b4 introduced a race
condition when accessing slow_io_work_running – it is being
increased and later decreased as part of the worker thread loop,
but was accessed with different mutexes during these operations.

This fixes the race condition by making sure both accesses
are protected through the global mutex of threadpool.c.

This fixes a number of flaky Node.js tests.

Refs: #1845
Refs: nodejs/reliability#18
Refs: nodejs/node#23089
Refs: nodejs/node#23067
Refs: nodejs/node#23066
Refs: nodejs/node#23219

[addaleax]

CI: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/1050/ (:heavy_check_mark: except infra failure on z/OS)
z/OS rebuild: https://ci.nodejs.org/job/libuv-test-commit-zos/1028/ (:heavy_check_mark:)
Node.js CI: https://ci.nodejs.org/job/node-test-commit/22032/ (:heavy_check_mark:)
Node.js stress test CI: https://ci.nodejs.org/job/node-stress-single-test/2060/ (:heavy_check_mark: not flaky on AIX; contains only failures of the form nodejs/reliability#18 (comment))

Would be cool to get a release out today if that’s possible? /cc @cjihrig

[gireeshpunathil]

libuv/src/threadpool.c

Line 115 in 28139bf

uv_cond_signal(&cond);

should we signal only if slow_work_thread_threshold() is met? else the idle workers get notified and go to dormancy for no reasons?

[addaleax]

should we signal only if slow_work_thread_threshold() is met? else the idle workers get notified and go to dormancy for no reasons?

I think you’re right, but I kind of don’t want to mix that improvement with a fix for a bug which has been haunting us for weeks 😄 Would you mind opening a separate PR?

[gireeshpunathil]

@addaleax - I am trying to reason out the problem, and the fix; please help me.

with 90891b4, the mutex was partially open for operations involving slow_io variable, and other threads were able to access those before one tread completes its work

implication to that is the slow_io workload is more carried out more than expected but seen as lesser than its defined threshold.

this PR fixes that by adjusting the slow_io work count through exclusive mutex so only one source of truth is visible.

But then it just takes us to the same point as the behavior prior to 90891b4 ? how the tests weer failing? I am unable to understand that. will you please clarify?

[addaleax]

@gireeshpunathil I’m not sure whether you’ve seen it, but is nodejs/node#23099 (comment) the answer to your question?

[gireeshpunathil]

yes, I just saw it, thanks.

[santigimeno]

@addaleax would it be possible adding a test that reproduces the issue you described (sending lots of fast DNS requests) ?

[addaleax]

@santigimeno I’m not sure – this seems to be platform-dependent in some way. In theory, just raising the CONCURRENT_COUNT in test-getaddrinfo.c should work, but it always passes for me regardless of how high I set it (as do the flaky Node.js tests).

[Trott]

Code and Learn is in less than a week and it would be really great to have Node.js CI green for it. I imagine a libuv release between now and then would be asking a lot, but maybe if this lands here, we can float it as a patch on our master branch?

[gireeshpunathil]

I was attempting for a stable recreate for the problem that PR addresses, but could not: my (JS) test case involved a unique domain name generator, a tight loop in which those are looked up, and the whole thing is repeated at intervals, with the interval frequency lying on the boundary of the speed of the resolution; expectation is being some looks up being piled up and eventual hang. Pileup happens, but it looks like the system recovers from it. I tried with a 2 CPU MAC and a 120 CPU Linux; but the pattern is same. So may be that slow_io_work_running though mis-computed, when work ceases, it comes back to normal? End result is that the tests just run slow; with issue indistinguishable from non-issue. It is just my theory.

[addaleax]

@gireeshpunathil I think the issue is more of a platform-dependent one in some way, so trying to reproduce on WIndows might be a better idea? (I couldn’t do so on Linux either.)

So may be that slow_io_work_running though mis-computed, when work ceases, it comes back to normal?

I don’t see how it could “magically” recover by itself…

[gireeshpunathil]

I mean - irrespective of the degree of race condition and data corruption, when one or more engaged workers return, those decrement the slow io work count, and there by allow some work to progress, though not optimally, but eventually completes all the pending work? Isn't it a possibility?

[addaleax]

Fwiw, I could reproduce this bug on Windows (somewhat flakily) by bumping CONCURRENT_COUNT to 1000 in test-getaddrinfo.c. I'm not sure whether there are any issues with adding that change to this PR, though, since the value does seem kind of high for a test that looks like it might do internet lookup?

when one or more engaged workers return, those decrement the slow io work count, and there by allow some work to progress

@gireeshpunathil As far as I could tell, the issue is that those decrements end up getting lost because of the race condition?

[addaleax]

@cjihrig I guess we want 48 hours before Code + Learn for the libuv upgrade PR, which would mean a release before Wednesday… would that be okay with you?

[cjihrig]

I'll make the release tomorrow (assuming this lands). There are actually a number of other Node bug fixes queued up for release. Proposal: #2024

[addaleax]

@cjihrig Thanks!

Went ahead and landed this in daf04e8. If somebody has strong feelings about the test I’ve mentioned above, feel free to let me know and I’ll gladly open a PR.