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.

Sign up for GitHub

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

Throw a separate exception if ESGetToken is uninitialized #30506

Merged
merged 3 commits into from
Sep 8, 2020
Merged

Throw a separate exception if ESGetToken is uninitialized #30506

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
67d33e4
Code format
makortel Jul 2, 2020
771ba07
Throw a separate exception if ESGetToken is uninitialized
makortel Jul 2, 2020
c62a716
Throw a differente exception if transition ID is set but index is not
makortel Sep 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 26 additions & 20 deletions FWCore/Framework/interface/EventSetupRecord.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,9 @@ namespace edm {

template <typename HolderT>
bool get(char const* iName, HolderT& iHolder) const {
if
UNLIKELY(requireTokens_) {
throwCalledGetWithoutToken(heterocontainer::className<typename HolderT::value_type>(), iName);
}
if UNLIKELY (requireTokens_) {
throwCalledGetWithoutToken(heterocontainer::className<typename HolderT::value_type>(), iName);
}
typename HolderT::value_type const* value = nullptr;
ComponentDescription const* desc = nullptr;
std::shared_ptr<ESHandleExceptionFactory> whyFailedFactory;
Expand All @@ -136,10 +135,9 @@ namespace edm {

template <typename HolderT>
bool get(ESInputTag const& iTag, HolderT& iHolder) const {
if
UNLIKELY(requireTokens_) {
throwCalledGetWithoutToken(heterocontainer::className<typename HolderT::value_type>(), iTag.data().c_str());
}
if UNLIKELY (requireTokens_) {
throwCalledGetWithoutToken(heterocontainer::className<typename HolderT::value_type>(), iTag.data().c_str());
}
typename HolderT::value_type const* value = nullptr;
ComponentDescription const* desc = nullptr;
std::shared_ptr<ESHandleExceptionFactory> whyFailedFactory;
Expand Down Expand Up @@ -207,26 +205,32 @@ namespace edm {
protected:
template <template <typename> typename H, typename T, typename R>
H<T> getHandleImpl(ESGetToken<T, R> const& iToken) const {
if
UNLIKELY(iToken.transitionID() != transitionID()) { throwWrongTransitionID(); }
assert(iToken.isInitialized());
if UNLIKELY (not iToken.isInitialized()) {
std::rethrow_exception(makeUninitializedTokenException(this->key(), DataKey::makeTypeTag<T>()));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 ...

if UNLIKELY (iToken.transitionID() != transitionID()) {
throwWrongTransitionID();
}
assert(getTokenIndices_);
//need to check token has valid index
if
UNLIKELY(not iToken.hasValidIndex()) { return invalidTokenHandle<H>(iToken); }
if UNLIKELY (not iToken.hasValidIndex()) {
return invalidTokenHandle<H>(iToken);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

... 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the data product is not available that would happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


auto proxyIndex = getTokenIndices_[iToken.index().value()];
if
UNLIKELY(proxyIndex.value() == std::numeric_limits<int>::max()) { return noProxyHandle<H>(iToken); }
if UNLIKELY (proxyIndex.value() == std::numeric_limits<int>::max()) {
return noProxyHandle<H>(iToken);
}

T const* value = nullptr;
ComponentDescription const* desc = nullptr;
std::shared_ptr<ESHandleExceptionFactory> whyFailedFactory;

impl_->getImplementation(value, proxyIndex, H<T>::transientAccessOnly, desc, whyFailedFactory, eventSetupImpl_);

if
UNLIKELY(not value) { return H<T>(std::move(whyFailedFactory)); }
if UNLIKELY (not value) {
return H<T>(std::move(whyFailedFactory));
}
return H<T>(value, desc);
}

Expand All @@ -251,8 +255,9 @@ namespace edm {
template <template <typename> typename H, typename T, typename R>
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>()); })};
return H<T>{makeESHandleExceptionFactory([key, transitionID = iToken.transitionID()] {
return makeInvalidTokenException(key, DataKey::makeTypeTag<T>(), transitionID);
})};
}

template <template <typename> typename H, typename T, typename R>
Expand All @@ -269,7 +274,8 @@ namespace edm {
ComponentDescription const*& iDesc,
bool iTransientAccessOnly) const;

static std::exception_ptr makeInvalidTokenException(EventSetupRecordKey const&, TypeTag const&);
static std::exception_ptr makeUninitializedTokenException(EventSetupRecordKey const&, TypeTag const&);
static std::exception_ptr makeInvalidTokenException(EventSetupRecordKey const&, TypeTag const&, unsigned int);
void throwWrongTransitionID() const;
static void throwCalledGetWithoutToken(const char* iTypeName, const char* iLabel);
// ---------- member data --------------------------------
Expand Down
15 changes: 13 additions & 2 deletions FWCore/Framework/src/EventSetupRecord.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ namespace edm {
iException.addContext(ost.str());
}

std::exception_ptr EventSetupRecord::makeInvalidTokenException(EventSetupRecordKey const& iRecordKey,
TypeTag const& iDataKey) {
std::exception_ptr EventSetupRecord::makeUninitializedTokenException(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()
Expand All @@ -80,6 +80,17 @@ namespace edm {
return std::make_exception_ptr(ex);
}

std::exception_ptr EventSetupRecord::makeInvalidTokenException(EventSetupRecordKey const& iRecordKey,
TypeTag const& iDataKey,
unsigned int iTransitionID) {
cms::Exception ex("InvalidESGetToken");
ex << "Attempted to get data using an invalid token of type ESGetToken<" << iDataKey.name() << ","
<< iRecordKey.name() << "> that had transition ID set (" << iTransitionID
<< ") but not the index.\n"
"This should not happen, please contact core framework developers.";
return std::make_exception_ptr(ex);
}

void EventSetupRecord::throwWrongTransitionID() const {
cms::Exception ex("ESGetTokenWrongTransition");
ex << "The transition ID stored in the ESGetToken does not match the\n"
Expand Down
9 changes: 9 additions & 0 deletions FWCore/Framework/test/eventsetup_t.cppunit.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,7 @@ namespace {
}

ESGetToken<edm::eventsetup::test::DummyData, edm::DefaultRecord> m_token;
ESGetToken<edm::eventsetup::test::DummyData, edm::DefaultRecord> m_tokenUninitialized;
};

class ConsumesProducer : public ESProducer {
Expand Down Expand Up @@ -763,6 +764,14 @@ void testEventsetup::getDataWithESGetTokenTest() {
true};
auto const& data = eventSetup.getData(consumer.m_token);
CPPUNIT_ASSERT(kGood.value_ == data.value_);
bool uninitializedTokenThrewException = false;
try {
(void)eventSetup.getData(consumer.m_tokenUninitialized);
} catch (cms::Exception& ex) {
uninitializedTokenThrewException = true;
CPPUNIT_ASSERT(ex.category() == "InvalidESGetToken");
}
CPPUNIT_ASSERT(uninitializedTokenThrewException);
}

{
Expand Down