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

Improve forking support #804

Merged
merged 21 commits into from Jan 10, 2024
Merged

Improve forking support #804

merged 21 commits into from Jan 10, 2024

Conversation

vickenty
Copy link
Contributor

@vickenty vickenty commented Dec 14, 2023

What does this PR do?

Use register_at_fork added in Python 3.7+ to transparently handle process fork when the client is using background threads.

Description of the Change

Forking support is introduced in several layers:

  • Per-instance hooks that can be called before and after fork() to disable and re-enable background threads and also flushes pending metrics.
  • A weakset is added that tracks all instances of DogStatsd (an opt-out is provided in case unforeseen complications arise).
  • Package level hooks that invoke fork hooks on all tracked instances.
  • On Python 3.7+, the package level hooks are automatically registered with the Python runtime via os.register_at_fork.

This approach is similar to one used in dd-trace-py for handling forks.

Alternate Designs

Possible Drawbacks

Verification Process

Additional Notes

Release Notes

Review checklist (to be filled by reviewers)

  • Feature or bug fix MUST have appropriate tests (unit, integration, etc...)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have one changelog/ label attached. If applicable it should have the backward-incompatible label attached.
  • PR should not have do-not-merge/ label attached.
  • If Applicable, issue must have kind/ and severity/ labels attached at least.

Instead of separate locks for buffering and sender modes
configuration, use the same lock.
Now that disable_buffering and pre_fork may both try to stop the flush thread
at the same time, attempt to stop an already stopped thread does not
indicate a bug in the client code.
Single field assignement is atomic according to Python FAQ, so we
don't need to protect reads and writes to self._queue as such. The
only place where we need a lock is when stopping the sender, to ensure
that the Stop marker is the last thing to be placed in there.

https://docs.python.org/3/faq/library.html#what-kinds-of-global-value-mutation-are-thread-safe
os.register_at_fork is not available on all python versions, notably
python 2.7. While we check for the availability explicitly, mypy
cannot see through that and complains.
@vickenty vickenty changed the title Improve forking support AMLII-1274 Improve forking support Dec 14, 2023
@vickenty vickenty changed the title AMLII-1274 Improve forking support Improve forking support Jan 4, 2024
@vickenty vickenty added the changelog/Added Added features results into a minor version bump label Jan 4, 2024
@vickenty vickenty marked this pull request as ready for review January 4, 2024 17:58
@vickenty vickenty requested review from a team as code owners January 4, 2024 17:58
The method is now thread safe, and can be used with forking applications.
Re-format parameter descriptions so they are rendered correctly in the
html docs.
gh123man
gh123man previously approved these changes Jan 9, 2024
skarimo
skarimo previously approved these changes Jan 10, 2024
@vickenty vickenty dismissed stale reviews from skarimo and gh123man via 35df4a1 January 10, 2024 16:40
@vickenty vickenty merged commit a51acab into master Jan 10, 2024
11 checks passed
@vickenty vickenty deleted the vickenty/fork branch January 10, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog/Added Added features results into a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants