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

TODO: Protect datadog_at_fork_blocks from at_fork monkey patch with lock #3831

Closed
ivoanjo opened this issue Aug 7, 2024 · 1 comment
Closed
Assignees
Labels
core Involves Datadog core libraries dev/refactor Involves refactoring existing components

Comments

@ivoanjo
Copy link
Member

ivoanjo commented Aug 7, 2024

As discussed in #3829 (comment) .

@ivoanjo ivoanjo added feature-request A request for a new feature or change to an existing one community Was opened by a community member labels Aug 7, 2024
@ivoanjo ivoanjo self-assigned this Aug 7, 2024
@ivoanjo ivoanjo added core Involves Datadog core libraries dev/refactor Involves refactoring existing components and removed feature-request A request for a new feature or change to an existing one community Was opened by a community member labels Aug 7, 2024
@ivoanjo
Copy link
Member Author

ivoanjo commented Aug 12, 2024

cc @p-datadog

I was going to pick this one up and add a mutex to the `AtForkMonkeyPatch, but actually I think it's no longer needed, especially after #3834 .

That is, in its current state we have:

      module AtForkMonkeyPatch
        AT_FORK_CHILD_BLOCKS = [] # rubocop:disable Style/MutableConstant Used to store blocks to run, mutable by design.
		
        #...

        def self.run_at_fork_blocks(stage)
          raise(ArgumentError, "Unsupported stage #{stage}") unless stage == :child

          AT_FORK_CHILD_BLOCKS.each(&:call)
        end

        def self.at_fork(stage, &block)
          raise(ArgumentError, "Unsupported stage #{stage}") unless stage == :child
          raise(ArgumentError, 'Missing block argument') unless block

          AT_FORK_CHILD_BLOCKS << block

          true
        end

...and:

  • Initialization is correct: it's done at require-time, and before requires are done there won't be any attempt to use the feature

  • There can't be concurrency between run_at_fork_blocks and at_fork: By design, run_at_fork_blocks is called by our monkey patch just after fork, and "just after fork" there won't exist other threads running that can be accessing this

  • Since we're the only users of this API, and we're not calling at_fork in background threads, there's also no concurrency between calls to at_fork.

Thus, while we could trivially add a Mutex here (I was half-way through doing it), I don't think we actually need one, given the current state of the code and how it's used.

I'm going to go ahead and close the issue, but ping me if you're not convinced by my hand-waving ;) :D

@ivoanjo ivoanjo closed this as completed Aug 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Involves Datadog core libraries dev/refactor Involves refactoring existing components
Projects
None yet
Development

No branches or pull requests

1 participant