Fix TypeError in os.fork() for garbage-collected processors#5453
Open
pranaysb wants to merge 11 commits into
Open
Fix TypeError in os.fork() for garbage-collected processors#5453pranaysb wants to merge 11 commits into
pranaysb wants to merge 11 commits into
Conversation
Assisted-by: Claude Opus 4.6
Assisted-by: Google Antigravity
Assisted-by: Google Antigravity
Assisted-by: Google Antigravity
Assisted-by: Google Antigravity
herin049
requested changes
Jul 23, 2026
|
|
||
| # string msg with no args - getMessage bypasses formatting and sets the string directly | ||
| logger.warning("a string with a percent-s: %s") | ||
| logger.warning("a string with a percent-s: %s") # pylint: disable=logging-too-few-args |
Contributor
There was a problem hiding this comment.
Why are we touching this?
Contributor
Author
There was a problem hiding this comment.
Ah, sorry about that! When I ran tox -e lint-opentelemetry-sdk locally to verify my changes, it failed on this exact line with E1206: Not enough arguments for logging format string (logging-too-few-args).
I added the inline suppression just to get the linting checks to pass, but I realize now I shouldn't be touching unrelated files. I have reverted this to keep the PR strictly focused on the os.fork() bug! (If the lint CI check ends up failing on this line again, just let me know how you'd prefer I handle it).
github-project-automation
Bot
moved this to Reviewed PRs that need fixes
in Python PR digest
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a TypeError: 'NoneType' object is not callable that can occur during os.fork() when at-fork callbacks are registered via os.register_at_fork using weakref.WeakMethod, and the underlying processor/reader instance has been garbage collected. The change updates the at-fork callback implementations to safely no-op when the weak reference has expired, aligning behavior with existing fork-handling patterns in the SDK.
Changes:
- Guard
os.register_at_fork(after_in_child=...)callbacks so weakly-referenced bound methods are only invoked when still alive. - Add fork-focused regression tests for
BatchProcessorandPeriodicExportingMetricReaderto ensure no unraisable exception is emitted tostderrafter GC. - Add a targeted pylint suppression in a log export test; add changelog entry.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| opentelemetry-sdk/src/opentelemetry/sdk/_shared_internal/init.py | Makes BatchProcessor’s at-fork callback safe when the weak method has expired. |
| opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/export/init.py | Makes PeriodicExportingMetricReader’s at-fork callback safe when the weak method has expired. |
| opentelemetry-sdk/tests/shared_internal/test_batch_processor.py | Adds a regression test that forks after GC and asserts no TypeError is printed to stderr. |
| opentelemetry-sdk/tests/metrics/test_periodic_exporting_metric_reader.py | Adds a regression test that forks after GC and asserts no TypeError is printed to stderr. |
| opentelemetry-sdk/tests/logs/test_export.py | Adds an inline pylint suppression for an intentional “too few args” logging call in a test. |
| .changelog/5453.fixed | Documents the bugfix in the changelog system. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+226
to
+231
| exporter = Mock() | ||
| processor = batch_processor_class(exporter) | ||
| processor.shutdown() | ||
| del processor | ||
| gc.collect() | ||
|
|
Comment on lines
+219
to
+222
| @unittest.skipUnless( | ||
| hasattr(os, "fork"), | ||
| "needs *nix", | ||
| ) |
Comment on lines
+239
to
+254
| old_fd = os.dup(sys.stderr.fileno()) | ||
| os.dup2(w_fd, sys.stderr.fileno()) | ||
| old_hook = sys.unraisablehook | ||
| sys.unraisablehook = sys.__unraisablehook__ | ||
|
|
||
| pid = os.fork() | ||
| if pid == 0: | ||
| os.close(r_fd) | ||
| # os.fork() has already run the at_fork hooks. | ||
| os._exit(0) | ||
|
|
||
| # Restore original stderr and hook in parent | ||
| sys.unraisablehook = old_hook | ||
| os.dup2(old_fd, sys.stderr.fileno()) | ||
| os.close(old_fd) | ||
| os.close(w_fd) |
Comment on lines
+319
to
+323
| processor = PeriodicExportingMetricReader(exporter) | ||
| processor.shutdown() | ||
| del processor | ||
| gc.collect() | ||
|
|
Comment on lines
+312
to
+313
| @pytest.mark.skipif(not hasattr(os, "fork"), reason="needs *nix") | ||
| def test_garbage_collected_processor_does_not_crash_on_fork(self): |
Comment on lines
+331
to
+346
| old_fd = os.dup(sys.stderr.fileno()) | ||
| os.dup2(w_fd, sys.stderr.fileno()) | ||
| old_hook = sys.unraisablehook | ||
| sys.unraisablehook = sys.__unraisablehook__ | ||
|
|
||
| pid = os.fork() | ||
| if pid == 0: | ||
| os.close(r_fd) | ||
| # os.fork() has already run the at_fork hooks. | ||
| os._exit(0) | ||
|
|
||
| # Restore original stderr and hook in parent | ||
| sys.unraisablehook = old_hook | ||
| os.dup2(old_fd, sys.stderr.fileno()) | ||
| os.close(old_fd) | ||
| os.close(w_fd) |
…t__.py Co-authored-by: Lukas Hering <40302054+herin049@users.noreply.github.com>
…rt/__init__.py Co-authored-by: Lukas Hering <40302054+herin049@users.noreply.github.com>
Assisted-by: Google Antigravity
Assisted-by: Google Antigravity
Assisted-by: Google Antigravity
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes
TypeError: 'NoneType' object is not callableinos.fork()when aBatchProcessororPeriodicExportingMetricReaderhas been garbage collected.The
after_in_childcallbacks registered viaos.register_at_forkduring initialization stored weak references toreinitmethods, but unconditionally invoked them without checking if the reference had resolved toNone. This PR adds a safe_after_in_childmethod that checks forNonebefore invoking the callback, matching the existing behavior ofConcurrentMultiSpanProcessor.Fixes #5452
Type of change
How Has This Been Tested?
Verified via the existing test suite and added specific unit tests verifying that garbage-collected processors do not crash the child process upon fork.
test_garbage_collected_processor_does_not_crash_on_forkintests/shared_internal/test_batch_processor.pytest_garbage_collected_processor_does_not_crash_on_forkintests/metrics/test_periodic_exporting_metric_reader.pyDoes This PR Require a Contrib Repo Change?
Checklist: