Throw a separate exception if ESGetToken is uninitialized #30506

makortel · 2020-07-02T15:43:11Z

PR description:

In #30482 it was noticed that a get() with uninitialized ESGetToken throws an exception complaining on incorrect transition ID. This is confusing and inconsistent with EDGetToken and EDPutToken. This PR changes EventSetupRecord::getHandleImpl() to first check if a token is initialized, and then check if the transition is correct.

In addition, scram b code-format lead to partial reformatting of EventSetupRecord.h (perhaps this file was missed in code formatting campaign after a clang version update or something), I included those changes in a separate commit.

PR validation:

Framework unit tests run.

cmsbuild · 2020-07-02T15:43:37Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-02T15:52:44Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30506/16700

This PR adds an extra 28KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/test/eventsetup_t.cppunit.cc modified in PR(s): [RFC] Clang tidy new checks #30495

cmsbuild · 2020-07-02T15:53:06Z

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

It involves the following packages:

FWCore/Framework

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

cms-bot commands are listed here

makortel · 2020-07-02T15:55:36Z

@cmsbuild, please test

cmsbuild · 2020-07-02T15:56:02Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-07-02-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/7615/console Started: 2020/07/02 17:56

makortel · 2020-07-02T15:57:02Z

FWCore/Framework/interface/EventSetupRecord.h

-        assert(iToken.isInitialized());
+        if UNLIKELY (not iToken.isInitialized()) {
+          std::rethrow_exception(makeInvalidTokenException(this->key(), DataKey::makeTypeTag<T>()));
+        }


@Dr15Jones I ended up re-using makeInvalidTokenException() because the exception message of that was about the same I had in mind. But that makes me ask ...

makortel · 2020-07-02T15:58:26Z

FWCore/Framework/interface/EventSetupRecord.h

-          UNLIKELY(not iToken.hasValidIndex()) { return invalidTokenHandle<H>(iToken); }
+        if UNLIKELY (not iToken.hasValidIndex()) {
+          return invalidTokenHandle<H>(iToken);
+        }


... if this check (which is the only caller of makeInvalidTokenException()) would still be useful? Can ESGetToken be constructed with a valid transition ID but invalid index?

If the data product is not available that would happen

Then the exception message of that appears to be confusing. invalidTokenHandle()

cmssw/FWCore/Framework/interface/EventSetupRecord.h

Lines 252 to 256 in cea1d1b

H<T> invalidTokenHandle(ESGetToken<T, R> const& iToken) const {

auto const key = this->key();

return H<T>{

makeESHandleExceptionFactory([key] { return makeInvalidTokenException(key, DataKey::makeTypeTag<T>()); })};

}

just leads to makeInvalidTokenException() that creates

cmssw/FWCore/Framework/src/EventSetupRecord.cc

Lines 73 to 81 in cea1d1b

std::exception_ptr EventSetupRecord::makeInvalidTokenException(EventSetupRecordKey const& iRecordKey,

TypeTag const& iDataKey) {

cms::Exception ex("InvalidESGetToken");

ex << "Attempted to get data using an invalid token of type ESGetToken<" << iDataKey.name() << ","

<< iRecordKey.name()

<< ">.\n"

"Please call consumes to properly initialize the token.";

return std::make_exception_ptr(ex);

}

that suggests to call consumes, which should be unrelated to (in)availability of the data product.

We concluded that a valid transition ID and invalid index should not happen, so I'll replace this exception message.

cmsbuild · 2020-07-02T19:21:24Z

+1
Tested at: 771ba07
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4d7028/7615/summary.html
CMSSW: CMSSW_11_2_X_2020-07-02-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-07-02T19:21:27Z

Comparison job queued.

cmsbuild · 2020-07-02T21:26:33Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 37
DQMHistoTests: Total histograms compared: 2784120
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2784069
DQMHistoTests: Total skipped: 50
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
Checked 154 log files, 17 edm output root files, 37 DQM output files

cmsbuild · 2020-09-04T16:40:21Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-09-04T16:45:13Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30506/18163

This PR adds an extra 32KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File FWCore/Framework/test/eventsetup_t.cppunit.cc modified in PR(s): Remove use of deprecated tbb::task_scheduler_init from FWCore #31313

cmsbuild · 2020-09-04T16:45:38Z

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

makortel · 2020-09-04T17:11:52Z

@cmsbuild, please test

cmsbuild · 2020-09-04T17:12:11Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-09-04-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/9129/console Started: 2020/09/04 19:13

cmsbuild · 2020-09-04T20:38:15Z

+1
Tested at: c62a716
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-4d7028/9129/summary.html
CMSSW: CMSSW_11_2_X_2020-09-04-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-09-04T20:38:18Z

Comparison job queued.

cmsbuild · 2020-09-04T22:04:45Z

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

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 0 differences found in the comparisons
DQMHistoTests: Total files compared: 35
DQMHistoTests: Total histograms compared: 2609667
DQMHistoTests: Total failures: 1
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2609644
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 34 files compared)
Checked 149 log files, 22 edm output root files, 35 DQM output files

makortel · 2020-09-08T13:37:53Z

+1

cmsbuild · 2020-09-08T13:38:14Z

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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

silviodonato · 2020-09-08T14:13:53Z

+1

makortel added 2 commits July 2, 2020 15:39

Code format

67d33e4

Throw a separate exception if ESGetToken is uninitialized

771ba07

cmsbuild added this to the CMSSW_11_2_X milestone Jul 2, 2020

cmsbuild added code-checks-pending comparison-pending core-pending orp-pending pending-signatures tests-pending labels Jul 2, 2020

makortel mentioned this pull request Jul 2, 2020

New Package - Phase2 L1 ParticleFlow #30482

Merged

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 2, 2020

cmsbuild added tests-started and removed tests-pending labels Jul 2, 2020

makortel commented Jul 2, 2020

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Jul 2, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jul 2, 2020

makortel changed the title ~~Throw a separate exception if ESgetToken is uninitialized~~ Throw a separate exception if ESGetToken is uninitialized Jul 3, 2020

cmsbuild mentioned this pull request Jul 3, 2020

[TEST ONLY][Do not merge] New clang tidy checks #30508

Closed

cmsbuild mentioned this pull request Jul 14, 2020

Fix Service access from ES modules #30688

Merged

cmsbuild added code-checks-pending comparison-pending tests-pending labels Sep 4, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 4, 2020

cmsbuild added tests-started and removed tests-pending labels Sep 4, 2020

cmsbuild added tests-approved and removed tests-started labels Sep 4, 2020

cmsbuild added comparison-available and removed comparison-pending labels Sep 4, 2020

cmsbuild added core-approved fully-signed and removed core-pending pending-signatures labels Sep 8, 2020

cmsbuild added orp-approved and removed orp-pending labels Sep 8, 2020

cmsbuild merged commit 6ee16df into cms-sw:master Sep 8, 2020

makortel deleted the esgettokenThrowUninitialized branch September 8, 2020 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Throw a separate exception if ESGetToken is uninitialized #30506

Throw a separate exception if ESGetToken is uninitialized #30506

makortel commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

makortel commented Jul 2, 2020

cmsbuild commented Jul 2, 2020 •

edited

Loading

makortel Jul 2, 2020

makortel Jul 2, 2020

Dr15Jones Jul 2, 2020

makortel Jul 2, 2020

makortel Sep 4, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

makortel commented Sep 4, 2020

cmsbuild commented Sep 4, 2020 •

edited

Loading

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

makortel commented Sep 8, 2020

cmsbuild commented Sep 8, 2020

silviodonato commented Sep 8, 2020

	H<T> invalidTokenHandle(ESGetToken<T, R> const& iToken) const {
	auto const key = this->key();
	return H<T>{
	makeESHandleExceptionFactory([key] { return makeInvalidTokenException(key, DataKey::makeTypeTag<T>()); })};
	}

	std::exception_ptr EventSetupRecord::makeInvalidTokenException(EventSetupRecordKey const& iRecordKey,
	TypeTag const& iDataKey) {
	cms::Exception ex("InvalidESGetToken");
	ex << "Attempted to get data using an invalid token of type ESGetToken<" << iDataKey.name() << ","
	<< iRecordKey.name()
	<< ">.\n"
	"Please call consumes to properly initialize the token.";
	return std::make_exception_ptr(ex);
	}

Throw a separate exception if ESGetToken is uninitialized #30506

Throw a separate exception if ESGetToken is uninitialized #30506

Conversation

makortel commented Jul 2, 2020

PR description:

PR validation:

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

makortel commented Jul 2, 2020

cmsbuild commented Jul 2, 2020 • edited Loading

makortel Jul 2, 2020

Choose a reason for hiding this comment

makortel Jul 2, 2020

Choose a reason for hiding this comment

Dr15Jones Jul 2, 2020

Choose a reason for hiding this comment

makortel Jul 2, 2020

Choose a reason for hiding this comment

makortel Sep 4, 2020

Choose a reason for hiding this comment

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Jul 2, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

makortel commented Sep 4, 2020

cmsbuild commented Sep 4, 2020 • edited Loading

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

cmsbuild commented Sep 4, 2020

makortel commented Sep 8, 2020

cmsbuild commented Sep 8, 2020

silviodonato commented Sep 8, 2020

cmsbuild commented Jul 2, 2020 •

edited

Loading

cmsbuild commented Sep 4, 2020 •

edited

Loading