Skip to content

[DI] Refactor mutex code guarding race condition #5728

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

Merged
merged 1 commit into from
May 23, 2025

Conversation

watson
Copy link
Collaborator

@watson watson commented May 14, 2025

What does this PR do?

When adding, removing or modifying a breakpoint there can be a race condtion if two Remove Config configs are received in quick succession. We already have a mutex to guard against this, but the code was in need of a refactor because:

  1. We had duplicate mutex code in two different files, where only one was needed
  2. We couldn't easily test the main mutex code in a unit test as it was added in the calling file and therefore not isolated to the function containing the race condition.

This PR fixes these issues by refactoring the code.

Motivation

Plugin Checklist

Additional Notes

This PR contains a lot of white space changes. Use "Hide whitespace" when reviewing for a better diff.

Copy link
Collaborator Author

watson commented May 14, 2025

Copy link

github-actions bot commented May 14, 2025

Overall package size

Self size: 9.41 MB
Deduped: 103.44 MB
No deduping: 103.96 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.5.1 | 29.73 MB | 29.73 MB | | @datadog/native-appsec | 8.5.2 | 19.33 MB | 19.34 MB | | @datadog/pprof | 5.8.0 | 12.55 MB | 12.92 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.4.0 | 2.77 MB | 5.42 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.8.0 | 1.21 MB | 1.21 MB | | import-in-the-middle | 1.13.1 | 117.64 kB | 839.26 kB | | source-map | 0.7.4 | 226 kB | 226 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | lru-cache | 7.18.3 | 133.92 kB | 133.92 kB | | pprof-format | 2.1.0 | 111.69 kB | 111.69 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 5.3.2 | 53.63 kB | 53.63 kB | | istanbul-lib-coverage | 3.2.0 | 29.34 kB | 29.34 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | dc-polyfill | 0.1.8 | 25.08 kB | 25.08 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.2 | 23.54 kB | 23.54 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.3 | 4.47 kB | 4.47 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

Copy link

codecov bot commented May 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.08%. Comparing base (0c27678) to head (1b4e0c7).
Report is 11 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5728      +/-   ##
==========================================
+ Coverage   78.69%   79.08%   +0.38%     
==========================================
  Files         500      517      +17     
  Lines       22978    23649     +671     
==========================================
+ Hits        18082    18702     +620     
- Misses       4896     4947      +51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@datadog-datadog-prod-us1
Copy link

datadog-datadog-prod-us1 bot commented May 14, 2025

Datadog Report

Branch report: watson/DEBUG-3872/clean-up-mutex-code
Commit report: 10b7006
Test service: dd-trace-js-integration-tests

✅ 0 Failed, 986 Passed, 0 Skipped, 15m 25.38s Total Time

@pr-commenter
Copy link

pr-commenter bot commented May 14, 2025

Benchmarks

Benchmark execution time: 2025-05-21 09:21:30

Comparing candidate commit 1b4e0c7 in PR branch watson/DEBUG-3872/clean-up-mutex-code with baseline commit 0c27678 in branch master.

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

@watson watson force-pushed the watson/DEBUG-3872/clean-up-mutex-code branch from 8c27bf3 to 1e64d50 Compare May 14, 2025 07:34
tylfin
tylfin previously approved these changes May 14, 2025
@BridgeAR BridgeAR marked this pull request as draft May 14, 2025 18:07
@watson watson changed the base branch from watson/DEBUG-3843/fix-race-condition to graphite-base/5728 May 17, 2025 10:30
@watson watson force-pushed the watson/DEBUG-3872/clean-up-mutex-code branch from 1e64d50 to 445a3a5 Compare May 17, 2025 10:31
@watson watson changed the base branch from graphite-base/5728 to watson/DEBUG-3843/fix-race-condition May 17, 2025 10:31
Base automatically changed from watson/DEBUG-3843/fix-race-condition to master May 19, 2025 08:34
@BridgeAR BridgeAR dismissed tylfin’s stale review May 19, 2025 08:34

The base branch was changed.

@watson watson force-pushed the watson/DEBUG-3872/clean-up-mutex-code branch from 445a3a5 to c6c8c5e Compare May 19, 2025 08:41
@watson watson marked this pull request as ready for review May 19, 2025 08:41
@watson watson force-pushed the watson/DEBUG-3872/clean-up-mutex-code branch from c6c8c5e to 6343c8e Compare May 19, 2025 08:42
When adding, removing or modifying a breakpoint there can be a race
condtion if two Remove Config configs are received in quick succession.
We already have a mutex to guard against this, but the code was in need
of a refactor because:

1. We had duplicate mutex code in two different files, where only one
   was needed
2. We couldn't easily test the main mutex code in a unit test as it was
   added in the calling file and therefore not isolated to the function
   containing the race condition.
@watson watson force-pushed the watson/DEBUG-3872/clean-up-mutex-code branch from 6343c8e to 1b4e0c7 Compare May 21, 2025 09:11
@BridgeAR BridgeAR merged commit 32dd20e into master May 23, 2025
593 of 596 checks passed
@BridgeAR BridgeAR deleted the watson/DEBUG-3872/clean-up-mutex-code branch May 23, 2025 12:20
dd-trace-js bot pushed a commit that referenced this pull request May 24, 2025
When adding, removing or modifying a breakpoint there can be a race
condtion if two Remove Config configs are received in quick succession.
We already have a mutex to guard against this, but the code was in need
of a refactor because:

1. We had duplicate mutex code in two different files, where only one
   was needed
2. We couldn't easily test the main mutex code in a unit test as it was
   added in the calling file and therefore not isolated to the function
   containing the race condition.
@dd-trace-js dd-trace-js bot mentioned this pull request May 24, 2025
dd-trace-js bot pushed a commit that referenced this pull request May 27, 2025
When adding, removing or modifying a breakpoint there can be a race
condtion if two Remove Config configs are received in quick succession.
We already have a mutex to guard against this, but the code was in need
of a refactor because:

1. We had duplicate mutex code in two different files, where only one
   was needed
2. We couldn't easily test the main mutex code in a unit test as it was
   added in the calling file and therefore not isolated to the function
   containing the race condition.
@dd-trace-js dd-trace-js bot mentioned this pull request May 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants