Skip to content

Make default stack size to be 1 MB in base16 #60811

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 18, 2025
Merged

Make default stack size to be 1 MB in base16 #60811

merged 4 commits into from
Apr 18, 2025

Conversation

@clintwar
Copy link
Contributor

@clintwar clintwar commented Mar 7, 2025

Make default stack size to be 1 MB in base16

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Description

Current default is expressed in decimal, however that is much larger than desired default.

Fixes #60762

Copilot AI review requested due to automatic review settings March 7, 2025 16:57
Copilot AI reviewed Mar 7, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@ghost ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Mar 7, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 7, 2025
adityamandaleeka
adityamandaleeka reviewed Mar 7, 2025
View reviewed changes
src/Servers/IIS/AspNetCoreModuleV2/InProcessRequestHandler/InProcessOptions.cpp Outdated
m_fSetCurrentDirectory = equals_ignore_case(find_element(handlerSettings, CS_ASPNETCORE_HANDLER_SET_CURRENT_DIRECTORY).value_or(L"true"), L"true");
m_fCallStartupHook = equals_ignore_case(find_element(handlerSettings, CS_ASPNETCORE_HANDLER_CALL_STARTUP_HOOK).value_or(L"true"), L"true");
m_strStackSize = find_element(handlerSettings, CS_ASPNETCORE_HANDLER_STACK_SIZE).value_or(L"1048576");
m_strStackSize = find_element(handlerSettings, CS_ASPNETCORE_HANDLER_STACK_SIZE).value_or(L"100000"); // this is correct as it should be in base16 1 MB
Copy link
Member

@adityamandaleeka adityamandaleeka Mar 7, 2025

Choose a reason for hiding this comment

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

I might actually also rename the variable to indicate the base.

@adityamandaleeka
Copy link
Member

adityamandaleeka commented Mar 7, 2025

Thanks for finding and fixing this!

@adityamandaleeka
Copy link
Member

adityamandaleeka commented Mar 10, 2025

Sorry for the confusion here, but I just realized that the default stack size should already be 1MB... why are we ever setting this explicitly if the config option isn't set? We should confirm that the default is 1MB and just not set it unless the user specified something else.

@clintwar
Copy link
Contributor Author

clintwar commented Mar 13, 2025

Default for .net core is actually 1.5 mb.

@clintwar
Copy link
Contributor Author

clintwar commented Mar 13, 2025

and actually for x64 I believe it is 4 MB

@adityamandaleeka
Copy link
Member

adityamandaleeka commented Mar 17, 2025

Just checked... it seems like it's 1.5 MB for both 32 and 64 bit on Windows, for .NET console apps. However, I'm not sure that also applies in the IIS case. Will confirm that.

@adityamandaleeka
Copy link
Member

adityamandaleeka commented Mar 21, 2025

Looks like in IIS we might have smaller stacks. With that in mind, this change seems fine (and strictly better than it was before).

@adityamandaleeka adityamandaleeka enabled auto-merge (squash) March 21, 2025 22:03
@clintwar
Copy link
Contributor Author

clintwar commented Mar 25, 2025

Do the checks need to be re-enqueued?

@BrennanConroy
Copy link
Member

BrennanConroy commented Mar 27, 2025

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Mar 27, 2025

Azure Pipelines successfully started running 3 pipeline(s).

@clintwar
Copy link
Contributor Author

clintwar commented Mar 28, 2025

I am not sure about those 3 other checks but they seem to have failed again.

This was referenced Mar 28, 2025
Closed
Closed
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 4, 2025
@clintwar
Copy link
Contributor Author

clintwar commented Apr 7, 2025

Seems like build is still failing all checks.

@BrennanConroy
Copy link
Member

BrennanConroy commented Apr 18, 2025

/azp run

@dotnet-policy-service dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Apr 18, 2025
@azure-pipelines
Copy link

azure-pipelines bot commented Apr 18, 2025

Azure Pipelines successfully started running 3 pipeline(s).

@wtgodbe wtgodbe disabled auto-merge April 18, 2025 19:30
@wtgodbe wtgodbe merged commit 9674103 into dotnet:main Apr 18, 2025
24 of 27 checks passed
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview4 milestone Apr 18, 2025
@clintwar clintwar deleted the users/clintwar/defaultStackSize branch April 22, 2025 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@adityamandaleeka adityamandaleeka adityamandaleeka approved these changes

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@JamesNK JamesNK Awaiting requested review from JamesNK JamesNK is a code owner

+2 more reviewers

@mgravell mgravell mgravell approved these changes

@apurvghai apurvghai apurvghai approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Milestone

10.0-preview4

Development

Successfully merging this pull request may close these issues.

stackSize override is not correct value format

7 participants

@clintwar @adityamandaleeka @BrennanConroy @mgravell @apurvghai @wtgodbe @halter73