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

feat: build-parameter for handler stack size #1049

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

supervacuus
Copy link
Collaborator

@supervacuus supervacuus commented Oct 8, 2024

This allows users to change the currently hard-coded handler stack size during the build.

Summary:

  • The default value is 64 KiB, equivalent to our current hard-coded value. This means for most users, everything stays the same.
  • For Windows, the following additional changes were introduced:
    • similar to the sigaltstack setup on the signal-handler side, we only configure the thread stack if no one else has done that before us.
    • sentry__reserve_thread_stack() is only called from the backend initialization for a static build. Dynamic libraries install a DllMain that sets the thread stack for each thread starting after our library is loaded.

I could imagine providing two more build parameters:

  • a CMake option to turn on/off the check for a previous stack configuration.
  • a CMake option to turn on/off the automatic configuration of each thread's crash stack (i.e., the DllMain).

For the sake of simplicity, I would like to defer this decision until we have a case where someone needs to configure the stack size and can not use these mechanisms.

Beyond the configurable stack size, most users prefer not to configure any of these subtle configurations themselves. Introducing any of these build parameters later can be done non-breakingly.

Copy link

github-actions bot commented Oct 8, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against ec0ff70

Copy link

codecov bot commented Oct 8, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 81.81%. Comparing base (65bfb62) to head (ec0ff70).
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1049      +/-   ##
==========================================
+ Coverage   81.80%   81.81%   +0.01%     
==========================================
  Files          53       53              
  Lines        6363     6363              
  Branches     1207     1207              
==========================================
+ Hits         5205     5206       +1     
  Misses       1045     1045              
+ Partials      113      112       -1     

@supervacuus
Copy link
Collaborator Author

supervacuus commented Oct 9, 2024

Hi @stima, would this be enough for you as an alternative to #1044? I thought about this again and would like to defer adding additional build parameters (beyond the one introduced here). With this approach, you can decrease the stack size without affecting other usage. If it is good enough for your use case, I will include this in the next release.

@stima
Copy link
Contributor

stima commented Oct 21, 2024

@supervacuus I don't think it's feasible to find a one-size-fits-all solution from an SDK perspective. Having the option to configure is better than not having it at all. However, in our specific case, we aren't aware of the stack size requirements for the third-party libraries we use, so we'll completely disable auto-attaching and set a guaranteed stack size for the threads we manage. Probably adding one more compilation flag (autoset: on/off) and some wrapper API around stack-size setting will provide more flexability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants