Skip to content
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

test: add test for effect of UV_THREADPOOL_SIZE #49165

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

tniessen
Copy link
Member

@tniessen tniessen commented Aug 14, 2023

This (not particularly elegant) native addon tests the effect of UV_THREADPOOL_SIZE on node-api. The test fails if Node.js allows more than UV_THREADPOOL_SIZE async tasks to run concurrently, or if it limits the number of concurrent async tasks to anything less than UV_THREADPOOL_SIZE.

The test schedules UV_THREADPOOL_SIZE + 1 async tasks and waits until UV_THREADPOOL_SIZE tasks are running concurrently. (If this never happens, the test times out.) All UV_THREADPOOL_SIZE tasks then wait for a second to see if the last task also starts running concurrently, which should not happen.

Admittedly, I hastily put this together and the code is sloppy. While having this test is good in any case, the main motivation is such that @anonrig can invoke the test from his own test in #48890.

@tniessen tniessen added test Issues and PRs related to the tests. libuv Issues and PRs related to the libuv dependency or the uv binding. node-api Issues and PRs related to the Node-API. labels Aug 14, 2023
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Aug 14, 2023
@tniessen tniessen mentioned this pull request Aug 14, 2023
11 tasks
@tniessen tniessen requested a review from anonrig August 14, 2023 13:41
This (not particularly elegant) native addon tests the effect of
UV_THREADPOOL_SIZE on node-api. The test fails if Node.js allows more
than UV_THREADPOOL_SIZE async tasks to run concurrently, or if it limits
the number of concurrent async tasks to anything less than
UV_THREADPOOL_SIZE.
@tniessen tniessen force-pushed the test-uv-threadpool-size branch from 25ac64c to c8edfb7 Compare August 14, 2023 13:43
@tniessen tniessen added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 14, 2023
@anonrig

This comment was marked as resolved.

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

Really good work. Thanks!

@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 14, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member

anonrig commented Aug 14, 2023

cc @nodejs/cpp-reviewers @nodejs/libuv

@anonrig anonrig added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. review wanted PRs that need reviews. labels Aug 14, 2023
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member

anonrig commented Aug 14, 2023

This works perfectly. I've tested it with the .env pull request.

@tniessen tniessen added the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 15, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Aug 16, 2023
@nodejs-github-bot nodejs-github-bot merged commit 556e95a into nodejs:main Aug 16, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 556e95a

UlisesGascon pushed a commit that referenced this pull request Sep 10, 2023
This (not particularly elegant) native addon tests the effect of
UV_THREADPOOL_SIZE on node-api. The test fails if Node.js allows more
than UV_THREADPOOL_SIZE async tasks to run concurrently, or if it limits
the number of concurrent async tasks to anything less than
UV_THREADPOOL_SIZE.

PR-URL: #49165
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@UlisesGascon UlisesGascon mentioned this pull request Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. libuv Issues and PRs related to the libuv dependency or the uv binding. needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. review wanted PRs that need reviews. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants