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

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.

Fix all pre-merge checks with AI

✨ 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.81%   +0.02%     
==========================================
  Files         183      184       +1     
  Lines        7823     7830       +7     
  Branches     1026     1026              
==========================================
+ Hits         6868     6876       +8     
  Misses        704      704              
+ Partials      251      250       -1

Flag	Coverage Δ
scripts	`86.35% <ø> (ø)`
unittests	`88.22% <100.00%> (+0.02%)`	⬆️

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 2 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...5218819. 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

Enable ROOT thread safety during FORM initialization

5218819

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

aolivier23 approved these changes Oct 16, 2026

View reviewed changes

pcanal reviewed Oct 19, 2026

View reviewed changes

pcanal approved these changes Oct 19, 2026

View reviewed changes

Navigation Menu

Repository navigation

Enable ROOT thread safety during FORM initialization - #989

Enable ROOT thread safety during FORM initialization#989
wwuoneway wants to merge 1 commit 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

Uh oh!

codecov Bot commented Oct 16, 2026

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!

Reviewers

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!

Uh oh!

codecov Bot commented Oct 16, 2026

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!

Reviewers

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