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

TrackletEventProcessor: properly initialise and check settings #30659

Conversation

fwyzard
Copy link
Contributor

@fwyzard fwyzard commented Jul 13, 2020

Initialise settings to nullptr in the constructor.
Check that settings is not nullptr before accessing it in the destructor.
Pass settings by reference to avoid the possibility of a null pointer.

Initialise settings_ to nullptr in the constructor.
Check that settings_ is not nullptr before accessing it in the destructor.
@fwyzard
Copy link
Contributor Author

fwyzard commented Jul 13, 2020

type bug-fix

@cmsbuild cmsbuild added this to the CMSSW_11_2_X milestone Jul 13, 2020
@fwyzard
Copy link
Contributor Author

fwyzard commented Jul 13, 2020

urgent

@cmsbuild
Copy link
Contributor

The code-checks are being triggered in jenkins.

@fwyzard
Copy link
Contributor Author

fwyzard commented Jul 13, 2020

@cmsbuild, please test

@fwyzard
Copy link
Contributor Author

fwyzard commented Jul 13, 2020

@skinnari FYI

@fwyzard
Copy link
Contributor Author

fwyzard commented Jul 13, 2020

This should fix a crash encountered while testing various Phase 2 workflow, where no runs and lumisections were processed.
That would leat the destructor of TrackletEventProcessor to try and access settings_ which had never been initialised, leading to a segmentation fault.

@cmsbuild
Copy link
Contributor

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30659/16941

@cmsbuild
Copy link
Contributor

cmsbuild commented Jul 13, 2020

The tests are being triggered in jenkins.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @fwyzard (Andrea Bocci) for master.

It involves the following packages:

L1Trigger/TrackFindingTracklet

@cmsbuild, @rekovic, @benkrikler can you please review it and eventually sign? Thanks.
@Martin-Grunewald this is something you requested to watch as well.
@silviodonato, @dpiparo you are the release manager for this.

cms-bot commands are listed here

@cmsbuild
Copy link
Contributor

+1
Tested at: 919094e
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f241d3/7912/summary.html
CMSSW: CMSSW_11_2_X_2020-07-13-1100
SCRAM_ARCH: slc7_amd64_gcc820

@cmsbuild
Copy link
Contributor

Comparison job queued.

@Sam-Harper
Copy link
Contributor

I guess this makes #30635 moot :)

@cmsbuild
Copy link
Contributor

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f241d3/7912/summary.html

Comparison Summary:

  • No significant changes to the logs found
  • Reco comparison results: 4 differences found in the comparisons
  • DQMHistoTests: Total files compared: 37
  • DQMHistoTests: Total histograms compared: 2787429
  • DQMHistoTests: Total failures: 5
  • DQMHistoTests: Total nulls: 1
  • DQMHistoTests: Total successes: 2787373
  • DQMHistoTests: Total skipped: 50
  • DQMHistoTests: Total Missing objects: 0
  • DQMHistoSizes: Histogram memory added: -0.004 KiB( 36 files compared)
  • DQMHistoSizes: changed ( 10224.0 ): -0.004 KiB MessageLogger/Warnings
  • Checked 154 log files, 17 edm output root files, 37 DQM output files

@fwyzard
Copy link
Contributor Author

fwyzard commented Jul 13, 2020

Looks like we are all fixing these same bugs ...

@silviodonato
Copy link
Contributor

merge

@rekovic
Copy link
Contributor

rekovic commented Jul 14, 2020

+1

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will be automatically merged.

@Sam-Harper
Copy link
Contributor

the memory leak remains here but its not a big deal. Also probably many other horrors lurking...

@fwyzard fwyzard deleted the initialise_TrackletEventProcessor_settings_11_2_x branch July 14, 2020 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants