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

Block remote config signals during ftp functions #2957

Merged
merged 4 commits into from
Nov 18, 2024

Conversation

bwoebi
Copy link
Collaborator

@bwoebi bwoebi commented Nov 15, 2024

These are sensitive to EINTR, so block the signal during their execution.

Fixes #2952.

@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.52%. Comparing base (6842245) to head (d5a6ed1).
Report is 1 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2957   +/-   ##
=========================================
  Coverage     66.52%   66.52%           
  Complexity     2739     2739           
=========================================
  Files           109      109           
  Lines         10831    10831           
=========================================
  Hits           7205     7205           
  Misses         3626     3626           
Flag Coverage Δ
tracer-php 66.52% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6842245...d5a6ed1. Read the comment docs.

@pr-commenter
Copy link

pr-commenter bot commented Nov 15, 2024

Benchmarks [ tracer ]

Benchmark execution time: 2024-11-18 13:31:06

Comparing candidate commit d5a6ed1 in PR branch bob/block-ftp-sigprocmask with baseline commit 6842245 in branch master.

Found 14 performance improvements and 1 performance regressions! Performance is the same for 163 metrics, 0 unstable metrics.

scenario:EmptyFileBench/benchEmptyFileBaseline

  • 🟩 execution_time [-463.695µs; -250.045µs] or [-13.936%; -7.515%]

scenario:EmptyFileBench/benchEmptyFileBaseline-opcache

  • 🟩 execution_time [-539.932µs; -331.608µs] or [-15.369%; -9.439%]

scenario:EmptyFileBench/benchEmptyFileOverhead

  • 🟩 execution_time [-633.831µs; -391.369µs] or [-16.974%; -10.481%]

scenario:EmptyFileBench/benchEmptyFileOverhead-opcache

  • 🟩 execution_time [-508.408µs; -303.612µs] or [-13.362%; -7.979%]

scenario:LaravelBench/benchLaravelBaseline

  • 🟩 execution_time [-596.411µs; -347.709µs] or [-5.494%; -3.203%]

scenario:LaravelBench/benchLaravelBaseline-opcache

  • 🟩 execution_time [-613.119µs; -333.021µs] or [-5.567%; -3.024%]

scenario:LaravelBench/benchLaravelOverhead

  • 🟩 execution_time [-1.328ms; -1.139ms] or [-10.592%; -9.091%]

scenario:LaravelBench/benchLaravelOverhead-opcache

  • 🟩 execution_time [-1.249ms; -0.995ms] or [-9.901%; -7.883%]

scenario:PDOBench/benchPDOBaseline

  • 🟥 execution_time [+10.175µs; +13.701µs] or [+5.639%; +7.593%]

scenario:SymfonyBench/benchSymfonyBaseline

  • 🟩 execution_time [-391.049µs; -267.411µs] or [-5.922%; -4.050%]

scenario:SymfonyBench/benchSymfonyBaseline-opcache

  • 🟩 execution_time [-479.490µs; -342.890µs] or [-7.094%; -5.073%]

scenario:SymfonyBench/benchSymfonyOverhead

  • 🟩 execution_time [-871.506µs; -803.014µs] or [-11.293%; -10.405%]

scenario:SymfonyBench/benchSymfonyOverhead-opcache

  • 🟩 execution_time [-913.512µs; -803.428µs] or [-11.613%; -10.214%]

scenario:WordPressBench/benchWordPressOverhead

  • 🟩 execution_time [-4.612ms; -4.262ms] or [-14.772%; -13.653%]

scenario:WordPressBench/benchWordPressOverhead-opcache

  • 🟩 execution_time [-4.454ms; -4.077ms] or [-14.241%; -13.036%]

@bwoebi bwoebi force-pushed the bob/block-ftp-sigprocmask branch 2 times, most recently from 5f0da0e to 19dcc2f Compare November 15, 2024 21:46

original_function(INTERNAL_FUNCTION_PARAM_PASSTHRU);

sigprocmask(SIG_UNBLOCK, &x, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

If my understanding is correct, this will re-emit at least one queued signal, meaning that you may not need to call ddtrace_check_for_new_config_now.

If invoking sigprocmask causes any pending signals to be unblocked, at least one of those signals is delivered to the process before sigprocmask returns. The order in which pending signals are delivered is not specified, but you can control the order explicitly by making multiple sigprocmask calls to unblock various signals one at a time. (reference)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting, that's really nice. Manpages for non-Linux (e.g. macos) say no such thing, so I'll #ifndef __linux__ this.

Done by deduplicating them.

Signed-off-by: Bob Weinand <[email protected]>
Handle case where the applictaion is stopped without other telemetry sent

This lead down to a path where the Stop action was actually enqueued without being ever causing a removal of the Application instances.

Signed-off-by: Bob Weinand <[email protected]>
Signed-off-by: Bob Weinand <[email protected]>
These are sensitive to EINTR, so block the signal during their execution.

Fixes #2952.

Signed-off-by: Bob Weinand <[email protected]>
@bwoebi bwoebi force-pushed the bob/block-ftp-sigprocmask branch from 9dc34b5 to ae21545 Compare November 18, 2024 12:22
@bwoebi bwoebi requested review from a team as code owners November 18, 2024 12:22
@bwoebi bwoebi changed the base branch from bob/live-debugger-dedup to master November 18, 2024 12:22
@pr-commenter
Copy link

pr-commenter bot commented Nov 18, 2024

Benchmarks [ appsec ]

Benchmark execution time: 2024-11-18 13:39:05

Comparing candidate commit d5a6ed1 in PR branch bob/block-ftp-sigprocmask with baseline commit 6842245 in branch master.

Found 5 performance improvements and 0 performance regressions! Performance is the same for 7 metrics, 0 unstable metrics.

scenario:LaravelBench/benchLaravelBaseline-appsec

  • 🟩 execution_time [-579.038µs; -383.722µs] or [-5.325%; -3.529%]

scenario:LaravelBench/benchLaravelOverhead-appsec

  • 🟩 execution_time [-1.386ms; -1.179ms] or [-9.374%; -7.973%]

scenario:SymfonyBench/benchSymfonyBaseline-appsec

  • 🟩 execution_time [-432.939µs; -351.341µs] or [-6.541%; -5.308%]

scenario:SymfonyBench/benchSymfonyOverhead-appsec

  • 🟩 execution_time [-1128.152µs; -772.128µs] or [-11.235%; -7.690%]

scenario:WordPressBench/benchWordPressOverhead-appsec

  • 🟩 execution_time [-4.445ms; -4.115ms] or [-13.128%; -12.154%]

@bwoebi bwoebi force-pushed the bob/block-ftp-sigprocmask branch from ae21545 to d5a6ed1 Compare November 18, 2024 13:02
@bwoebi bwoebi merged commit f7328a8 into master Nov 18, 2024
699 of 745 checks passed
@bwoebi bwoebi deleted the bob/block-ftp-sigprocmask branch November 18, 2024 14:06
@github-actions github-actions bot added this to the 1.6.0 milestone Nov 18, 2024
@bwoebi bwoebi modified the milestones: 1.6.0, 1.5.0 Nov 18, 2024
@iamluc iamluc mentioned this pull request Dec 17, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants