Skip to content

Commit 2225e32

Browse files
Dr15JonesDr15Jones
committed
Avoid possible exception while processing exception case
- Reuse ModuleSignalSentry for this case. - Keep past behavior where exception in post signal during begin also resulted in module end not being called.
1 parent 650fde8 commit 2225e32

File tree

4 files changed

+192
-180
lines changed

4 files changed

+192
-180
lines changed

‎FWCore/Framework/interface/maker/ModuleSignalSentry.h‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
#ifndef FWCore_Framework_ModuleSignalSentry_h
2+
#define FWCore_Framework_ModuleSignalSentry_h
3+
4+
#include "FWCore/Utilities/interface/ConvertException.h"
5+
6+
namespace edm {
7+
class ActivityRegistry;
8+
class ModuleCallingContext;
9+
10+
template <typename T>
11+
class ModuleSignalSentry {
12+
public:
13+
ModuleSignalSentry(ActivityRegistry* a,
14+
typename T::Context const* context,
15+
ModuleCallingContext const* moduleCallingContext)
16+
: a_(a), context_(context), moduleCallingContext_(moduleCallingContext) {}
17+
18+
~ModuleSignalSentry() {
19+
// This destructor does nothing unless we are unwinding the
20+
// the stack from an earlier exception (a_ will be null if we are
21+
// are not). We want to report the earlier exception and ignore any
22+
// addition exceptions from the post module signal.
23+
CMS_SA_ALLOW try {
24+
if (a_) {
25+
T::postModuleSignal(a_, context_, moduleCallingContext_);
26+
}
27+
} catch (...) {
28+
}
29+
}
30+
void preModuleSignal() {
31+
if (a_) {
32+
try {
33+
convertException::wrap([this]() { T::preModuleSignal(a_, context_, moduleCallingContext_); });
34+
} catch (cms::Exception& ex) {
35+
ex.addContext("Handling pre module signal, likely in a service function immediately before module method");
36+
throw;
37+
}
38+
}
39+
}
40+
void postModuleSignal() {
41+
if (a_) {
42+
auto temp = a_;
43+
// Setting a_ to null informs the destructor that the signal
44+
// was already run and that it should do nothing.
45+
a_ = nullptr;
46+
try {
47+
convertException::wrap([this, temp]() { T::postModuleSignal(temp, context_, moduleCallingContext_); });
48+
} catch (cms::Exception& ex) {
49+
ex.addContext("Handling post module signal, likely in a service function immediately after module method");
50+
throw;
51+
}
52+
}
53+
}
54+
55+
private:
56+
ActivityRegistry* a_; // We do not use propagate_const because the registry itself is mutable.
57+
typename T::Context const* context_;
58+
ModuleCallingContext const* moduleCallingContext_;
59+
};
60+
} // namespace edm
61+
62+
#endif

‎FWCore/Framework/interface/maker/Worker.h‎

Lines changed: 1 addition & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ the worker is reset().
2626
#include "FWCore/MessageLogger/interface/ExceptionMessages.h"
2727
#include "FWCore/Framework/interface/TransitionInfoTypes.h"
2828
#include "FWCore/Framework/interface/maker/WorkerParams.h"
29+
#include "FWCore/Framework/interface/maker/ModuleSignalSentry.h"
2930
#include "FWCore/Framework/interface/ExceptionActions.h"
3031
#include "FWCore/Framework/interface/ModuleContextSentry.h"
3132
#include "FWCore/Framework/interface/OccurrenceTraits.h"
@@ -607,61 +608,6 @@ namespace edm {
607608
bool shouldTryToContinue_ = false;
608609
bool beginSucceeded_ = false;
609610
};
610-
611-
namespace {
612-
template <typename T>
613-
class ModuleSignalSentry {
614-
public:
615-
ModuleSignalSentry(ActivityRegistry* a,
616-
typename T::Context const* context,
617-
ModuleCallingContext const* moduleCallingContext)
618-
: a_(a), context_(context), moduleCallingContext_(moduleCallingContext) {}
619-
620-
~ModuleSignalSentry() {
621-
// This destructor does nothing unless we are unwinding the
622-
// the stack from an earlier exception (a_ will be null if we are
623-
// are not). We want to report the earlier exception and ignore any
624-
// addition exceptions from the post module signal.
625-
CMS_SA_ALLOW try {
626-
if (a_) {
627-
T::postModuleSignal(a_, context_, moduleCallingContext_);
628-
}
629-
} catch (...) {
630-
}
631-
}
632-
void preModuleSignal() {
633-
if (a_) {
634-
try {
635-
convertException::wrap([this]() { T::preModuleSignal(a_, context_, moduleCallingContext_); });
636-
} catch (cms::Exception& ex) {
637-
ex.addContext("Handling pre module signal, likely in a service function immediately before module method");
638-
throw;
639-
}
640-
}
641-
}
642-
void postModuleSignal() {
643-
if (a_) {
644-
auto temp = a_;
645-
// Setting a_ to null informs the destructor that the signal
646-
// was already run and that it should do nothing.
647-
a_ = nullptr;
648-
try {
649-
convertException::wrap([this, temp]() { T::postModuleSignal(temp, context_, moduleCallingContext_); });
650-
} catch (cms::Exception& ex) {
651-
ex.addContext("Handling post module signal, likely in a service function immediately after module method");
652-
throw;
653-
}
654-
}
655-
}
656-
657-
private:
658-
ActivityRegistry* a_; // We do not use propagate_const because the registry itself is mutable.
659-
typename T::Context const* context_;
660-
ModuleCallingContext const* moduleCallingContext_;
661-
};
662-
663-
} // namespace
664-
665611
namespace workerhelper {
666612
template <>
667613
class CallImpl<OccurrenceTraits<EventPrincipal, BranchActionStreamBegin>> {

‎FWCore/Framework/src/ModuleRegistryUtilities.cc‎

Lines changed: 129 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
#include "FWCore/Framework/interface/ModuleRegistryUtilities.h"
22
#include "FWCore/Framework/interface/ModuleRegistry.h"
3-
#include "FWCore/Common/interface/ProcessBlockHelperBase.h"
4-
#include "FWCore/Framework/interface/ESRecordsToProductResolverIndices.h"
3+
#include "FWCore/Framework/interface/maker/ModuleSignalSentry.h"
54
#include "FWCore/ServiceRegistry/interface/ActivityRegistry.h"
65
#include "FWCore/Utilities/interface/make_sentry.h"
76
#include "FWCore/Utilities/interface/ExceptionCollector.h"
@@ -38,6 +37,103 @@ namespace edm {
3837
});
3938
}
4039

40+
namespace {
41+
template <typename F>
42+
void captureExceptionAndContinue(std::exception_ptr& iExcept, ModuleCallingContext const& iContext, F&& iF) {
43+
try {
44+
convertException::wrap(std::forward<F&&>(iF));
45+
} catch (cms::Exception& newException) {
46+
if (not iExcept) {
47+
edm::exceptionContext(newException, iContext);
48+
iExcept = std::current_exception();
49+
}
50+
}
51+
}
52+
53+
template <typename F>
54+
void captureExceptionsAndContinue(ExceptionCollector& iCollector, ModuleCallingContext const& iContext, F&& iF) {
55+
try {
56+
convertException::wrap(std::forward<F&&>(iF));
57+
} catch (cms::Exception& ex) {
58+
edm::exceptionContext(ex, iContext);
59+
iCollector.addException(ex);
60+
}
61+
}
62+
template <typename F>
63+
void captureExceptionsAndContinue(ExceptionCollector& iCollector,
64+
std::mutex& iMutex,
65+
ModuleCallingContext const& iContext,
66+
F&& iF) {
67+
try {
68+
convertException::wrap(std::forward<F&&>(iF));
69+
} catch (cms::Exception& ex) {
70+
edm::exceptionContext(ex, iContext);
71+
std::lock_guard guard(iMutex);
72+
iCollector.addException(ex);
73+
}
74+
}
75+
76+
class ModuleBeginJobTraits {
77+
public:
78+
using Context = GlobalContext;
79+
static void preModuleSignal(ActivityRegistry* activityRegistry,
80+
GlobalContext const*,
81+
ModuleCallingContext const* moduleCallingContext) {
82+
activityRegistry->preModuleBeginJobSignal_(*moduleCallingContext->moduleDescription());
83+
}
84+
static void postModuleSignal(ActivityRegistry* activityRegistry,
85+
GlobalContext const*,
86+
ModuleCallingContext const* moduleCallingContext) {
87+
activityRegistry->postModuleBeginJobSignal_(*moduleCallingContext->moduleDescription());
88+
}
89+
};
90+
91+
class ModuleEndJobTraits {
92+
public:
93+
using Context = GlobalContext;
94+
static void preModuleSignal(ActivityRegistry* activityRegistry,
95+
GlobalContext const*,
96+
ModuleCallingContext const* moduleCallingContext) {
97+
activityRegistry->preModuleEndJobSignal_(*moduleCallingContext->moduleDescription());
98+
}
99+
static void postModuleSignal(ActivityRegistry* activityRegistry,
100+
GlobalContext const*,
101+
ModuleCallingContext const* moduleCallingContext) {
102+
activityRegistry->postModuleEndJobSignal_(*moduleCallingContext->moduleDescription());
103+
}
104+
};
105+
106+
class ModuleBeginStreamTraits {
107+
public:
108+
using Context = StreamContext;
109+
static void preModuleSignal(ActivityRegistry* activityRegistry,
110+
StreamContext const* streamContext,
111+
ModuleCallingContext const* moduleCallingContext) {
112+
activityRegistry->preModuleBeginStreamSignal_(*streamContext, *moduleCallingContext);
113+
}
114+
static void postModuleSignal(ActivityRegistry* activityRegistry,
115+
StreamContext const* streamContext,
116+
ModuleCallingContext const* moduleCallingContext) {
117+
activityRegistry->postModuleBeginStreamSignal_(*streamContext, *moduleCallingContext);
118+
}
119+
};
120+
121+
class ModuleEndStreamTraits {
122+
public:
123+
using Context = StreamContext;
124+
static void preModuleSignal(ActivityRegistry* activityRegistry,
125+
StreamContext const* streamContext,
126+
ModuleCallingContext const* moduleCallingContext) {
127+
activityRegistry->preModuleEndStreamSignal_(*streamContext, *moduleCallingContext);
128+
}
129+
static void postModuleSignal(ActivityRegistry* activityRegistry,
130+
StreamContext const* streamContext,
131+
ModuleCallingContext const* moduleCallingContext) {
132+
activityRegistry->postModuleEndStreamSignal_(*streamContext, *moduleCallingContext);
133+
}
134+
};
135+
136+
} // namespace
41137
void runBeginJobForModules(GlobalContext const& iGlobalContext,
42138
ModuleRegistry& iModuleRegistry,
43139
edm::ActivityRegistry& iActivityRegistry,
@@ -51,24 +147,15 @@ namespace edm {
51147
ModuleCallingContext mcc(&holder->moduleDescription());
52148
//Also sets a thread local
53149
ModuleContextSentry mccSentry(&mcc, pc);
54-
try {
55-
convertException::wrap([&]() {
56-
// if exception happens, before or during beginJob, add to beginJobFailedForModule
57-
auto failedSentry = make_sentry(&beginJobFailedForModule,
58-
[&](auto* failed) { failed->push_back(holder->moduleDescription().id()); });
59-
auto sentry = make_sentry(&holder->moduleDescription(), [&](auto const* description) {
60-
iActivityRegistry.postModuleBeginJobSignal_(*description);
61-
});
62-
iActivityRegistry.preModuleBeginJobSignal_(holder->moduleDescription());
63-
holder->beginJob();
64-
failedSentry.release();
65-
});
66-
} catch (cms::Exception& ex) {
67-
if (!exceptionPtr) {
68-
edm::exceptionContext(ex, mcc);
69-
exceptionPtr = std::current_exception();
70-
}
71-
}
150+
captureExceptionAndContinue(exceptionPtr, mcc, [&]() {
151+
ModuleSignalSentry<ModuleBeginJobTraits> signalSentry(&iActivityRegistry, &iGlobalContext, &mcc);
152+
auto failedSentry = make_sentry(&beginJobFailedForModule,
153+
[&](auto* failed) { failed->push_back(holder->moduleDescription().id()); });
154+
signalSentry.preModuleSignal();
155+
holder->beginJob();
156+
signalSentry.postModuleSignal();
157+
failedSentry.release();
158+
});
72159
});
73160
if (exceptionPtr) {
74161
std::rethrow_exception(exceptionPtr);
@@ -92,18 +179,12 @@ namespace edm {
92179
ModuleCallingContext mcc(&holder->moduleDescription());
93180
//Also sets a thread local
94181
ModuleContextSentry mccSentry(&mcc, pc);
95-
try {
96-
convertException::wrap([&]() {
97-
auto sentry = make_sentry(&holder->moduleDescription(), [&](auto const* description) {
98-
iActivityRegistry.postModuleEndJobSignal_(*description);
99-
});
100-
iActivityRegistry.preModuleEndJobSignal_(holder->moduleDescription());
101-
holder->endJob();
102-
});
103-
} catch (cms::Exception& ex) {
104-
edm::exceptionContext(ex, mcc);
105-
collector.addException(ex);
106-
}
182+
captureExceptionsAndContinue(collector, mcc, [&]() {
183+
ModuleSignalSentry<ModuleEndJobTraits> signalSentry(&iActivityRegistry, &iGlobalContext, &mcc);
184+
signalSentry.preModuleSignal();
185+
holder->endJob();
186+
signalSentry.postModuleSignal();
187+
});
107188
});
108189
}
109190

@@ -119,24 +200,15 @@ namespace edm {
119200
ModuleCallingContext mcc(&holder->moduleDescription());
120201
//Also sets a thread local
121202
ModuleContextSentry mccSentry(&mcc, pc);
122-
try {
123-
convertException::wrap([&]() {
124-
auto sentry = make_sentry(&mcc, [&](auto const* context) {
125-
iActivityRegistry.postModuleBeginStreamSignal_(iStreamContext, *context);
126-
});
127-
// if exception happens, before or during beginStream, add to beginStreamFailedForModule
128-
auto failedSentry = make_sentry(&beginStreamFailedForModule,
129-
[&](auto* failed) { failed->push_back(holder->moduleDescription().id()); });
130-
iActivityRegistry.preModuleBeginStreamSignal_(iStreamContext, mcc);
131-
holder->beginStream(iStreamContext.streamID());
132-
failedSentry.release();
133-
});
134-
} catch (cms::Exception& ex) {
135-
if (!exceptionPtr) {
136-
edm::exceptionContext(ex, mcc);
137-
exceptionPtr = std::current_exception();
138-
}
139-
}
203+
captureExceptionAndContinue(exceptionPtr, mcc, [&]() {
204+
auto failedSentry = make_sentry(&beginStreamFailedForModule,
205+
[&](auto* failed) { failed->push_back(holder->moduleDescription().id()); });
206+
ModuleSignalSentry<ModuleBeginStreamTraits> signalSentry(&iActivityRegistry, &iStreamContext, &mcc);
207+
signalSentry.preModuleSignal();
208+
holder->beginStream(iStreamContext.streamID());
209+
signalSentry.postModuleSignal();
210+
failedSentry.release();
211+
});
140212
});
141213
if (exceptionPtr) {
142214
std::rethrow_exception(exceptionPtr);
@@ -161,19 +233,13 @@ namespace edm {
161233
ModuleCallingContext mcc(&holder->moduleDescription());
162234
//Also sets a thread local
163235
ModuleContextSentry mccSentry(&mcc, pc);
164-
try {
165-
convertException::wrap([&]() {
166-
auto sentry = make_sentry(
167-
&mcc, [&](auto const* mc) { iActivityRegistry.postModuleEndStreamSignal_(iStreamContext, *mc); });
168-
iActivityRegistry.preModuleEndStreamSignal_(iStreamContext, mcc);
169-
holder->endStream(iStreamContext.streamID());
170-
});
171-
} catch (cms::Exception& ex) {
172-
edm::exceptionContext(ex, mcc);
173-
std::lock_guard<std::mutex> collectorLock(collectorMutex);
174-
collector.addException(ex);
175-
}
236+
captureExceptionsAndContinue(collector, collectorMutex, mcc, [&]() {
237+
ModuleSignalSentry<ModuleEndStreamTraits> signalSentry(&iActivityRegistry, &iStreamContext, &mcc);
238+
signalSentry.preModuleSignal();
239+
holder->endStream(iStreamContext.streamID());
240+
signalSentry.postModuleSignal();
241+
});
176242
});
177243
}
178244

179-
} // namespace edm
245+
} // namespace edm

0 commit comments

Comments
 (0)