Fix thread safety issues in SwitchProducer ProductResolvers #28686

makortel · 2019-12-27T20:01:33Z

PR description:

This PR fixes three thread safety issues in SwitchProducer ProductResolvers that I realized while addressing #28680

SwitchBaseProductResolver::prefetchRequested_ is now re-set to false in resetProductData_()
- Current behavior (without re-setting) could lead to starvation if there are many consumers (in Tasks) for the SwitchProducer products. This indeed happens with enhanced tests (multiple consumers, multiple threads)
Call ProductData::unsafe_setWrapper() in the prefetching phase as suggested by @Dr15Jones in Thread safety issue in SwitchBaseProductResolver::resolveProductImpl #28680
- The ordering of the tasks ensures that exactly one task is accessing the ProductData at the time of the call, after which the tasks for the product consumers are spawned.
SwitchBaseProductResolver::waitingTasks_ is reset in resetProductData_()
- Current behavior (without reset) leads to product consumer tasks being spawned too early from 2nd event onwards

In addition this PR

Moves putProduct_() from SwitchBaseProductResolver to SwitchProducerProductResolver
- The putProduct_() should never be called from SwitchAliasProductResolver (and now throws an exception)
Moves the call to updateProvenance() to a proper place in SwitchProducerProductResolver
- The earlier placement looked suspicious (why update if got an exception), and the problem in the provenance tracking was confirmed by enhancing the tests to check the provenance information also for the test configuration where the SwitchProcucer is in a Path

Fixes #28680.

PR validation:

Code compiles, unit tests run.

cmsbuild · 2019-12-27T20:02:21Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-12-27T20:08:38Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28686/13257

This PR adds an extra 40KB to repository

Code check has found code style and quality issues which could be resolved by applying following patch(s)

code-checks:
https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28686/13257/code-checks.patch
e.g. curl https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28686/13257/code-checks.patch | patch -p1
You can also run scram build code-checks to apply code checks directly

…oductResolver putProduct_() should never be called fro SwitchAliasProductResolver, which means that the infrastructure to support putProduct_() is not common for the two. Moving the function to SwitchProducerProductResolver makes the intentions more clear. Also remove a useless return from updateProvenance().

Found by checking provenance from SwitchProducer-in-Path as well

cmsbuild · 2019-12-27T20:14:01Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-12-27T20:20:49Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28686/13258

This PR adds an extra 40KB to repository

cmsbuild · 2019-12-27T20:22:25Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

FWCore/Framework
FWCore/Integration

@cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@wddgit this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos, @silviodonato you are the release manager for this.

cms-bot commands are listed here

cmsbuild · 2019-12-27T20:22:26Z

A new Pull Request was created by @makortel (Matti Kortelainen) for master.

It involves the following packages:

FWCore/Framework
FWCore/Integration

@cmsbuild, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks.
@wddgit this is something you requested to watch as well.
@davidlange6, @slava77, @fabiocos, @silviodonato you are the release manager for this.

cms-bot commands are listed here

makortel · 2019-12-27T20:22:42Z

@cmsbuild, please test

cmsbuild · 2019-12-27T20:22:57Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4107/console Started: 2019/12/27 21:24

cmsbuild · 2019-12-30T17:37:31Z

The code-checks are being triggered in jenkins.

cmsbuild · 2019-12-30T17:44:43Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28686/13263

This PR adds an extra 52KB to repository

cmsbuild · 2019-12-30T17:46:21Z

Pull request #28686 was updated. @cmsbuild, @smuzaffar, @Dr15Jones can you please check and sign again.

makortel · 2019-12-30T17:46:44Z

@cmsbuild, please test

cmsbuild · 2019-12-30T17:46:46Z

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/4110/console Started: 2019/12/30 18:47

cmsbuild · 2019-12-30T19:03:14Z

+1
Tested at: d521f7f
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-16228b/4110/summary.html
CMSSW: CMSSW_11_1_X_2019-12-30-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2019-12-30T19:03:17Z

Comparison job queued.

cmsbuild · 2019-12-30T20:52:02Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2815513
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2815166
DQMHistoTests: Total skipped: 346
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 147 log files, 16 edm output root files, 34 DQM output files

Dr15Jones · 2019-12-30T20:52:44Z

+1

cmsbuild · 2019-12-30T20:53:07Z

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 now be reviewed by the release team before it's merged. @davidlange6, @slava77, @smuzaffar, @fabiocos, @silviodonato (and backports should be raised in the release meeting by the corresponding L2)

fabiocos · 2019-12-31T11:28:30Z

+1

Reset SwitchBaseProductResolver::prefetchRequested_

113bcf3

makortel mentioned this pull request Dec 27, 2019

Thread safety issue in SwitchBaseProductResolver::resolveProductImpl #28680

Closed

cmsbuild added this to the CMSSW_11_1_X milestone Dec 27, 2019

cmsbuild added code-checks-pending comparison-pending core-pending orp-pending pending-signatures tests-pending labels Dec 27, 2019

cmsbuild added code-checks-rejected and removed code-checks-pending labels Dec 27, 2019

makortel added 5 commits December 27, 2019 21:11

Call unsafe_setWrapper() in prefetching phase

f16eb0c

Reset WaitingTaskList

1d81dac

Run SwitchProducer tests in multiple threads, add multiple consumers

ccf415e

Call updateProvenance() properly in SwitchProducerProductResolver

9890e15

Found by checking provenance from SwitchProducer-in-Path as well

makortel force-pushed the fixSwitchThreadSafety branch from dec71aa to 9890e15 Compare December 27, 2019 20:13

cmsbuild added code-checks-pending and removed code-checks-rejected labels Dec 27, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 27, 2019

cmsbuild removed the tests-pending label Dec 27, 2019

cmsbuild added code-checks-pending comparison-pending tests-pending and removed tests-approved labels Dec 30, 2019

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 30, 2019

cmsbuild added tests-started and removed tests-pending labels Dec 30, 2019

cmsbuild added tests-approved and removed tests-started labels Dec 30, 2019

cmsbuild added comparison-available and removed comparison-pending labels Dec 30, 2019

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Dec 30, 2019

cmsbuild added orp-approved and removed orp-pending labels Dec 31, 2019

cmsbuild merged commit 8f299bc into cms-sw:master Dec 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix thread safety issues in SwitchProducer ProductResolvers #28686

Fix thread safety issues in SwitchProducer ProductResolvers #28686

makortel commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

makortel commented Dec 27, 2019

cmsbuild commented Dec 27, 2019 •

edited

cmsbuild commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

makortel commented Dec 30, 2019

cmsbuild commented Dec 30, 2019 •

edited

cmsbuild commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

Dr15Jones commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

fabiocos commented Dec 31, 2019

Fix thread safety issues in SwitchProducer ProductResolvers #28686

Fix thread safety issues in SwitchProducer ProductResolvers #28686

Conversation

makortel commented Dec 27, 2019

PR description:

PR validation:

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

cmsbuild commented Dec 27, 2019

makortel commented Dec 27, 2019

cmsbuild commented Dec 27, 2019 • edited

cmsbuild commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

makortel commented Dec 30, 2019

cmsbuild commented Dec 30, 2019 • edited

cmsbuild commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

Dr15Jones commented Dec 30, 2019

cmsbuild commented Dec 30, 2019

fabiocos commented Dec 31, 2019

cmsbuild commented Dec 27, 2019 •

edited

cmsbuild commented Dec 30, 2019 •

edited