Enable ROOT thread safety during FORM initialization by wwuoneway · Pull Request #989 · Framework-R-D/phlex

wwuoneway · 2026-10-16T20:18:59Z

##Summary

Call ROOT::EnableThreadSafety() exactly once, when FORM constructs its storage layer, before any concurrent event processing starts.

Phlex schedules graph nodes on TBB worker threads, so FORM's containers — and any other node touching ROOT — reach ROOT's global TClass and streamer state concurrently. ROOT does not serialize that state unless thread safety is explicitly enabled.

Without the existing workaround in phlex-examples, for example, the reproducer showed intermittent corruption and crashes.

This also relates to the threading work mentioned in #840.
#840 proposes "write-path mutexes that follow the existing read-side mutex pattern". This is a prerequisite for #840, not an alternative, and #840 acceptance criteria should probably say so.

Code: Added a std::call_once-guarded helper that calls ROOT::EnableThreadSafety() once.
Initialization: FORM calls the helper when it creates a storage reader or writer. The helper does nothing when ROOT storage is not compiled in.
Build system: Added the ROOT thread-safety implementation to the root_storage source list.
Tests: No test results were provided.

coderabbitai · 2026-10-16T20:19:18Z

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Source mode: Auto
Repository: Unavailable
Branch: Unavailable
File: No configuration file from this repository applied
Commit: Unavailable
Layers: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: 4ab634d2-3c01-4296-a2bf-4d08cdd31a1d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Advanced
Run ID: adf492ab-0f69-4eed-8ff2-b3e58b3dd0cf

📥 Commits

Reviewing files that changed from the base of the PR and between fb7fb00 and 5218819.

📒 Files selected for processing: 7

form/root_storage/CMakeLists.txt
form/root_storage/root_thread_safety.cpp
form/root_storage/root_thread_safety.hpp
form/storage/factories.cpp
form/storage/factories.hpp
form/storage/storage_reader.cpp
form/storage/storage_writer.cpp

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📜 Recent review details

⏰ Context from checks skipped due to timeout: 4

GitHub Check: Analyze cpp with CodeQL
GitHub Check: build (gcc, none)
GitHub Check: coverage
GitHub Check: clang-tidy-check

🧰 Additional context used

📚 Code guidelines: 1

.github/copilot-instructions.md — unknown

📓 Path-based instructions: 1

Source excerpt: Use clang-format tool for all C++ code (VS Code auto-formats on save)

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Files:

form/storage/storage_writer.cpp
form/storage/storage_reader.cpp
form/storage/factories.cpp
form/root_storage/root_thread_safety.hpp
form/storage/factories.hpp
form/root_storage/root_thread_safety.cpp

🪛 Cppcheck (2.22.0)

form/storage/factories.cpp

[style] 35-35: The function 'ensure_backend_thread_safety' is never used.

(unusedFunction)

form/root_storage/root_thread_safety.cpp

[style] 11-11: The function 'ensure_root_thread_safety' is never used.

(unusedFunction)

🔇 Additional comments: 7

form/root_storage/root_thread_safety.hpp: 1

3-13: LGTM!

form/root_storage/root_thread_safety.cpp: 1

11-15: LGTM!

form/root_storage/CMakeLists.txt: 1

10-10: LGTM!

form/storage/factories.hpp: 1

14-14: LGTM!

form/storage/factories.cpp: 1

19-19: LGTM!

Also applies to: 35-41

form/storage/storage_reader.cpp: 1

193-193: LGTM!

form/storage/storage_writer.cpp: 1

61-61: LGTM!

📝 Walkthrough

Storage reader and writer creation now ensure backend thread safety before construction. With ROOT storage enabled, the backend helper calls a new one-time initializer for ROOT::EnableThreadSafety().

Changes: ROOT thread-safety initialization

Layer / File(s)	Summary
Add one-time ROOT initialization `form/root_storage/root_thread_safety.hpp`, `form/root_storage/root_thread_safety.cpp`, `form/root_storage/CMakeLists.txt`	A new function invokes `ROOT::EnableThreadSafety()` once, using `std::call_once`. The ROOT storage source list includes its implementation.
Initialize before storage construction `form/storage/factories.hpp`, `form/storage/factories.cpp`, `form/storage/storage_reader.cpp`, `form/storage/storage_writer.cpp`	The backend helper calls the ROOT initializer when ROOT storage is enabled and does nothing otherwise. Reader and writer creation call the helper before constructing their storage objects.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: knoepfel

Merge Risk: ⚪ Minimal · up to 52188

Supported storage construction enables ROOT thread safety once before creating readers or writers. No actionable merge-blocking risk was identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 6 files. (1 skipped: 1 …	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the primary change: enabling ROOT thread safety during FORM initialization.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 6 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-10-16T20:28:13Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main     #989      +/-   ##
==========================================
+ Coverage   87.79%   87.89%   +0.09%     
==========================================
  Files         183      184       +1     
  Lines        7823     7829       +6     
  Branches     1026     1026              
==========================================
+ Hits         6868     6881      +13     
+ Misses        704      699       -5     
+ Partials      251      249       -2

Flag	Coverage Δ
amd64	`88.28% <100.00%> (?)`
arm64	`87.89% <100.00%> (?)`
scripts	`?`
unittests	`?`

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

Files with missing lines	Coverage Δ
form/root_storage/root_thread_safety.cpp	`100.00% <100.00%> (ø)`
form/storage/factories.cpp	`100.00% <100.00%> (ø)`
form/storage/storage_reader.cpp	`80.90% <100.00%> (+0.08%)`	⬆️
form/storage/storage_writer.cpp	`81.66% <100.00%> (+0.31%)`	⬆️

... and 3 files with indirect coverage changes

Continue to review full report in Codecov by Harness.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

aolivier23

LGTM

pcanal · 2026-10-19T16:55:31Z

+    static std::once_flag enabled;
+    std::call_once(enabled, [] { ROOT::EnableThreadSafety(); });


Side note ROOT::EnableThreadSafety() is safe to call multiple time.

Good point — agreed, ROOT::EnableThreadSafety() is idempotent and safe to call repeatedly ("only the first call does work"). I had that noted locally but left it out of the final commit message.

I am not sure what you mean to change. The current code work whether ROOT::EnableThreadSafety() is idempotent and is redundant (beside a tad bit of potential performance improvement) since it is idempotent.

If , ROOT::EnableThreadSafety() is idempotent FORM doesn't need to ensure its only called once, but leave things to ROOT. We just don't want to cal it a billion trillion times

Thanks. Would you prefer that I remove std::call_once and std::once_flag and call ROOT::EnableThreadSafety() directly, leaving repeated-call handling to ROOT? The function would still be called from the appropriate initialization path, rather than a hot path.

I would, but up to you.

Done — thanks for the feedback. Please let me know if you have any further concerns.

…::EnableThreadSafety() to handle repeated calls instead of enforcing one-time initialization in FORM.

Enable ROOT thread safety during FORM initialization

5218819

wwuoneway requested review from aolivier23 and gemmeren October 16, 2026 20:33

aolivier23 previously approved these changes Oct 16, 2026

View reviewed changes

pcanal reviewed Oct 19, 2026

View reviewed changes

pcanal previously approved these changes Oct 19, 2026

View reviewed changes

wwuoneway mentioned this pull request Oct 22, 2026

FORM: locate every cell through the index container #1007

Merged

coderabbitai Bot mentioned this pull request Oct 22, 2026

FORM: make the read path thread-safe for multi-product form_source #1008

Open

Remove call_once from ROOT thread safety initialization; Rely on ROOT…

12795e0

…::EnableThreadSafety() to handle repeated calls instead of enforcing one-time initialization in FORM.

wwuoneway dismissed stale reviews from pcanal and aolivier23 via 12795e0 October 23, 2026 18:40

Navigation Menu

Repository navigation

Enable ROOT thread safety during FORM initialization - #989

Enable ROOT thread safety during FORM initialization#989
wwuoneway wants to merge 2 commits into
Framework-R-D:mainfrom
wwuoneway:form-root-thread-safety

wwuoneway commented Oct 16, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading

Review skipped

Uh oh!

codecov Bot commented Oct 16, 2026 •

edited

Loading

Uh oh!

aolivier23 left a comment

Uh oh!

pcanal Oct 19, 2026

Uh oh!

wwuoneway Oct 19, 2026

Uh oh!

pcanal Oct 19, 2026

Uh oh!

gemmeren Oct 19, 2026

Uh oh!

wwuoneway Oct 23, 2026

Uh oh!

gemmeren Oct 23, 2026

Uh oh!

wwuoneway Oct 23, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		static std::once_flag enabled;
		std::call_once(enabled, [] { ROOT::EnableThreadSafety(); });

Conversation

wwuoneway commented Oct 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

codecov Bot commented Oct 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aolivier23 left a comment

Choose a reason for hiding this comment

Uh oh!

pcanal Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

wwuoneway Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

pcanal Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

gemmeren Oct 19, 2026

Choose a reason for hiding this comment

Uh oh!

wwuoneway Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

gemmeren Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

wwuoneway Oct 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wwuoneway commented Oct 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 16, 2026 •

edited

Loading

codecov Bot commented Oct 16, 2026 •

edited

Loading