Skip to content

inspector: more conservative minimum stack size - #27855

Closed
bnoordhuis wants to merge 2 commits into
nodejs:masterfrom
bnoordhuis:min-thread-stack-size
Closed

inspector: more conservative minimum stack size#27855
bnoordhuis wants to merge 2 commits into
nodejs:masterfrom
bnoordhuis:min-thread-stack-size

Conversation

@bnoordhuis

Copy link
Copy Markdown
Member

PTHREAD_STACK_MIN is 2 KB with musl, which is too small to safely
receive signals. PTHREAD_STACK_MIN + MINSIGSTKSZ is 8 KB on arm64,
which is the musl architecture with the biggest MINSIGSTKSZ so let's
use that as a lower bound and let's quadruple it just in case.

This coincidentally also lets us remove a FreeBSD-specific workaround.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels May 24, 2019
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

Related: libuv/libuv#2310

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

Re-run to check if the freebsd failures are flakes or regressions: https://ci.nodejs.org/job/node-test-pull-request/23324/

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@bnoordhuis
bnoordhuisforce-pushed the min-thread-stack-size branch from da49258 to e943c71CompareMay 25, 2019 14:59
@bnoordhuis

Copy link
Copy Markdown
MemberAuthor

I guess freebsd doesn't like small stacks... I decided to leave well enough alone.

Comment threadsrc/inspector_agent.cc Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I guess this doesn’t change anything compared to the current situation, but should we do something like libuv/libuv@38bb0f5 (use PTHREAD_STACK_MIN only inside an #ifdef)?

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fair point. Addressed in the fix-up commit.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

PTHREAD_STACK_MIN is 2 KB with musl, which is too small to safely
receive signals. PTHREAD_STACK_MIN + MINSIGSTKSZ is 8 KB on arm64,
which is the musl architecture with the biggest MINSIGSTKSZ so let's
use that as a lower bound and let's quadruple it just in case.
@bnoordhuis
bnoordhuisforce-pushed the min-thread-stack-size branch from e943c71 to ee68f56CompareJune 1, 2019 06:59
@bnoordhuisbnoordhuis added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 1, 2019
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@Trott

Trott commented Jun 2, 2019

Copy link
Copy Markdown
Member

Landed in 5539d7e

@TrottTrott closed this Jun 2, 2019
Trott pushed a commit to Trott/io.js that referenced this pull request Jun 2, 2019
PTHREAD_STACK_MIN is 2 KB with musl, which is too small to safely
receive signals. PTHREAD_STACK_MIN + MINSIGSTKSZ is 8 KB on arm64,
which is the musl architecture with the biggest MINSIGSTKSZ so let's
use that as a lower bound and let's quadruple it just in case.
PR-URL: nodejs#27855
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Jun 3, 2019
PTHREAD_STACK_MIN is 2 KB with musl, which is too small to safely
receive signals. PTHREAD_STACK_MIN + MINSIGSTKSZ is 8 KB on arm64,
which is the musl architecture with the biggest MINSIGSTKSZ so let's
use that as a lower bound and let's quadruple it just in case.
PR-URL: #27855
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targostargos mentioned this pull request Jun 3, 2019
Flarna pushed a commit to dynatrace-oss-contrib/node that referenced this pull request Jun 4, 2020
PTHREAD_STACK_MIN is 2 KB with musl, which is too small to safely
receive signals. PTHREAD_STACK_MIN + MINSIGSTKSZ is 8 KB on arm64,
which is the musl architecture with the biggest MINSIGSTKSZ so let's
use that as a lower bound and let's quadruple it just in case.
Backport-PR-URL: nodejs#33720
PR-URL: nodejs#27855
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
richardlau pushed a commit that referenced this pull request Jul 1, 2020
PTHREAD_STACK_MIN is 2 KB with musl, which is too small to safely
receive signals. PTHREAD_STACK_MIN + MINSIGSTKSZ is 8 KB on arm64,
which is the musl architecture with the biggest MINSIGSTKSZ so let's
use that as a lower bound and let's quadruple it just in case.
Backport-PR-URL: #33720
PR-URL: #27855
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Eugene Ostroukhov <eostroukhov@google.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@richardlaurichardlau mentioned this pull request Jul 2, 2020
4 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs 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++.inspectorIssues and PRs related to the V8 inspector protocol

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@bnoordhuis@nodejs-github-bot@Trott@eugeneo@jasnell@addaleax@cjihrig