Skip to content

Conversation

@pinkenburg
Copy link
Contributor

@pinkenburg pinkenburg commented Jan 15, 2026

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • [ X] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work for users)
  • Requiring change in macros repository (Please provide links to the macros pull request in the last section)
  • I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

What kind of change does this PR introduce? (Bug fix, feature, ...)

pythia8 was messing with our cout state which then modifies all subsequent printouts. In order to prevent subsystem code to be able to do this - fun4all now restores the saved cout state after every call to subsystem code

TODOs (if applicable)

Links to other PRs in macros and calibration repositories (if applicable)

Pull Request Summary: Preserve cout State

Motivation / Context

Pythia8 and other subsystem code can modify the global std::cout formatting state (flags, precision, width), which can unexpectedly change subsequent printed output across subsystems. This PR ensures Fun4All saves and restores std::cout formatting around subsystem calls so transient changes inside subsystems do not leak into the rest of the application.

Key Changes

  • Fun4AllServer:
    • Added member std::ios m_saved_cout_state to store cout formatting.
    • Save initial cout state in InitAll().
    • Restore cout formatting (std::cout.copyfmt(m_saved_cout_state)) after subsystem calls in registerSubsystem, process_event, BeginRun, EndRun, End, and related paths.
    • Minor refactor: inline initialization of histomanagername.
  • PHPythia8:
    • Preserve and restore local std::cout formatting around Pythia8 initialization and event generation (init and process_event) to avoid altering global formatting.
  • No public API/signature changes detected.

Potential Risk Areas

  • IO formatting scope: Only iostream formatting state (flags, precision, fill, width, etc.) is preserved via copyfmt. Other global state (streambuf redirection, tied streams, locales) is not affected.
  • Exception safety: Restoration relies on explicit copyfmt calls after subsystem invocations; if subsystems throw and Fun4All does not reach the restore call, formatting may still be left altered. Current code catches exceptions in many places but not necessarily every exceptional control flow; restoration could be missed in some paths.
  • Thread safety: std::cout is a global resource; concurrent subsystem use from multiple threads is not addressed by this change.
  • Performance: Frequent copyfmt on every subsystem call and per-event may have minor overhead at very high event rates.
  • Duplicated declaration risk: earlier raw summary noted duplicate member declarations in header; current checked header shows a single member—verify no accidental duplicate remained in other branches/edits.

Possible Future Improvements

  • Implement an RAII guard (scope-based) to capture and restore stream state automatically and exception-safely.
  • Consider also protecting std::cerr and other global streams if affected.
  • Document expectations for subsystem behavior (do not modify global stream state) in developer guidelines.
  • Profile performance impact if running at extreme event rates; optimize if necessary.
  • If multi-threading is used, add synchronization or thread-local handling for stream state.

Note: AI-generated summaries can be incorrect or incomplete—please review the diff and tests carefully and use best judgment when accepting changes.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 15, 2026

📝 Walkthrough

Walkthrough

Added systematic preservation and restoration of std::cout formatting state around Pythia8 initialization and event processing, and across multiple Fun4AllServer lifecycle points; introduced a member to hold the saved stream state (appears duplicated in header—verify).

Changes

Cohort / File(s) Summary
Pythia8 Output State Management
generators/PHPythia8/PHPythia8.cc
Save std::cout format state around Init() (before/after m_Pythia8->init()) and around process_event() (save at start, restore on error returns and on normal exit). Replaced a verbose comment with a shorter inline note.
Fun4All Server State Management & Initialization
offline/framework/fun4all/Fun4AllServer.cc
Save/restore std::cout formatting at multiple lifecycle points: after subsystem registration, after event processing, after BeginRun, EndRun, and End. Minor change: histomanagername initialized inline.
Fun4All Server Member Variable Addition
offline/framework/fun4all/Fun4AllServer.h
Added std::ios m_saved_cout_state{nullptr}; to store saved std::cout formatting. Note: The declaration appears in both public and protected sections (duplicate)—please confirm intended access and remove the unintended duplicate.

Sequence Diagram(s)

(omitted — changes are stream-state preservation points and do not introduce a new multi-component control flow requiring a sequence diagram)

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bd1ae49 and 04d5376.

📒 Files selected for processing (1)
  • generators/PHPythia8/PHPythia8.cc
🚧 Files skipped from review as they are similar to previous changes (1)
  • generators/PHPythia8/PHPythia8.cc

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 64138d4 and bd1ae49.

📒 Files selected for processing (3)
  • generators/PHPythia8/PHPythia8.cc
  • offline/framework/fun4all/Fun4AllServer.cc
  • offline/framework/fun4all/Fun4AllServer.h
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cc,cpp,cxx,c}

⚙️ CodeRabbit configuration file

**/*.{cc,cpp,cxx,c}: Prioritize correctness, memory safety, error handling, and thread-safety.
Flag hidden global state, non-const singletons, and unclear lifetime assumptions.

Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.

Files:

  • offline/framework/fun4all/Fun4AllServer.cc
  • generators/PHPythia8/PHPythia8.cc
**/*.{h,hpp,hxx,hh}

⚙️ CodeRabbit configuration file

**/*.{h,hpp,hxx,hh}: Focus on API clarity/stability, ownership semantics (RAII), and avoiding raw new/delete.
If interfaces change, ask for compatibility notes and any needed downstream updates.

Only raise Critical or Major findings. Do not post minor style, formatting, naming, or “nice-to-have” refactors.

Files:

  • offline/framework/fun4all/Fun4AllServer.h
🧠 Learnings (2)
📚 Learning: 2026-01-08T18:39:48.023Z
Learnt from: emclaughlin2
Repo: sPHENIX-Collaboration/coresoftware PR: 4098
File: offline/QA/Calorimeters/CaloValid.cc:270-270
Timestamp: 2026-01-08T18:39:48.023Z
Learning: In C++ code that parses or traverses data nodes, implement a dual-key fallback when looking up a GL1Packet-like node: first try the version-specific key (e.g., "14001"), then fall back to the legacy/general key (e.g., "GL1Packet"). This ensures compatibility with multiple data versions where the node naming may have changed over time. Apply this pattern to files under the offline/ Calorimeters area (and similar modules) to maintain data-structure resilience across versions.

Applied to files:

  • offline/framework/fun4all/Fun4AllServer.cc
📚 Learning: 2026-01-10T07:54:57.021Z
Learnt from: silas-gross
Repo: sPHENIX-Collaboration/coresoftware PR: 4100
File: generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h:93-108
Timestamp: 2026-01-10T07:54:57.021Z
Learning: In HepMCParticleTrigger (generators/Herwig/HepMCTrigger/HepMCParticleTrigger.h), the SetEta* and SetAbsEta* methods intentionally share the same storage variables (_theEtaHigh and _theEtaLow) to emulate the Pythia trigger behavior. This design pattern should be maintained for consistency with existing trigger implementations.

Applied to files:

  • generators/PHPythia8/PHPythia8.cc
🔇 Additional comments (7)
offline/framework/fun4all/Fun4AllServer.h (1)

146-147: No concerns with this member addition.

generators/PHPythia8/PHPythia8.cc (1)

126-133: No concerns with this change.

offline/framework/fun4all/Fun4AllServer.cc (5)

248-248: No concerns with this change.


580-580: No concerns with this change.


904-904: No concerns with this change.


1098-1098: No concerns with this change.


1151-1151: No concerns with this change.

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.

Comment on lines +131 to +133
m_saved_cout_state.copyfmt(std::cout); // save current state
Fun4AllMonitoring::instance()->Snapshot("StartUp");
std::string histomanagername;
histomanagername = Name() + "HISTOS";
std::string histomanagername = Name() + "HISTOS";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "Fun4AllServer.cc" -type f

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 122


🏁 Script executed:

# Get the file size first
wc -l ./offline/framework/fun4all/Fun4AllServer.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 127


🏁 Script executed:

# Read the relevant section around lines 131-133
sed -n '125,145p' ./offline/framework/fun4all/Fun4AllServer.cc

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 945


🏁 Script executed:

# Search for m_saved_cout_state usage throughout the file
rg "m_saved_cout_state" ./offline/framework/fun4all/Fun4AllServer.cc -n -A 2 -B 2

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 1273


🏁 Script executed:

# Look for the class definition to understand the member variable
rg "class Fun4AllServer" ./offline/framework/fun4all/ -A 30

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 2821


🏁 Script executed:

# Search for subsystem call sites and lifecycle methods
rg "subsystem->Init|subsystem->InitRun|subsystem->End" ./offline/framework/fun4all/Fun4AllServer.cc -n -B 2 -A 2

Repository: sPHENIX-Collaboration/coresoftware

Length of output: 313


Save cout formatting state immediately before each subsystem call, not just at startup.

m_saved_cout_state is captured once during InitAll() but restored at multiple lifecycle points (after subsystem Init(), process_event(), and BeginRunSubsystem() calls). If user code or loaded macros adjust std::cout formatting after server construction, the restore operations will reset those changes back to the startup state. Capture the current state immediately before each restore point to preserve user-applied formatting while still isolating subsystem-induced changes.

Refresh m_saved_cout_state before each major lifecycle entry where a restore will later occur.

@sphenix-jenkins-ci
Copy link

Build & test report

Report for commit bd1ae49728ee04de2524280630f1e9956fd76d7d:
Jenkins passed


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@sphenix-jenkins-ci
Copy link

Build & test report

Report for commit 04d53767137b4b03b84e5aeef28e911b6f304b1b:
Jenkins passed


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant