ThresholdTimer: use weak_ptr in async callback
Capture weak_ptr to sensor in ThresholdTimer async callback.
which is used to detect destructed sensor in the callback.
Removed raw sensor pointer inside the ThresholdTimer.
When ADCSensor is destructed, ThresholdTimer is cancelled,
but already queued timer callback is not removed and can be
executed after the sensor is desctructed.
This change prevents accessing the dangling raw sensor pointer
and fixes the occasional ADCSensor service crash while trying to
run createSensors. Any services use ThresholdTimer have
the same issue.
Tested:
ipmitool power cycle 1000 times, ADCSensors get deleted and recreated
for every cycle without crashing.
Signed-off-by: AppaRao Puli <apparao.puli@linux.intel.com>
Signed-off-by: Zhikui Ren <zhikui.ren@intel.com>
Change-Id: Ibee7a58a2605992554fb33f4c34ebee502eb38d6
diff --git a/include/Thresholds.hpp b/include/Thresholds.hpp
index df55182..af63f72 100644
--- a/include/Thresholds.hpp
+++ b/include/Thresholds.hpp
@@ -59,8 +59,7 @@
struct ThresholdTimer
{
- ThresholdTimer(boost::asio::io_service& ioService, Sensor* sensor) :
- io(ioService), sensor(sensor)
+ ThresholdTimer(boost::asio::io_service& ioService) : io(ioService)
{}
bool hasActiveTimer(const Threshold& threshold, bool assert)
@@ -98,12 +97,12 @@
}
}
- void startTimer(const Threshold& threshold, bool assert,
+ void startTimer(const std::weak_ptr<Sensor>& weakSensor,
+ const Threshold& threshold, bool assert,
double assertValue);
boost::asio::io_service& io;
std::list<TimerPair> timers;
- Sensor* sensor;
};
bool parseThresholdsFromConfig(
@@ -129,6 +128,7 @@
void updateThresholds(Sensor* sensor);
// returns false if a critical threshold has been crossed, true otherwise
bool checkThresholds(Sensor* sensor);
-void checkThresholdsPowerDelay(Sensor* sensor, ThresholdTimer& thresholdTimer);
+void checkThresholdsPowerDelay(const std::weak_ptr<Sensor>& weakSensor,
+ ThresholdTimer& thresholdTimer);
} // namespace thresholds
diff --git a/src/ADCSensor.cpp b/src/ADCSensor.cpp
index ef7a90a..e77261a 100644
--- a/src/ADCSensor.cpp
+++ b/src/ADCSensor.cpp
@@ -61,7 +61,7 @@
inputDev(io, open(path.c_str(), O_RDONLY)), waitTimer(io), path(path),
scaleFactor(scaleFactor),
sensorPollMs(static_cast<unsigned int>(pollRate * 1000)),
- bridgeGpio(std::move(bridgeGpio)), thresholdTimer(io, this)
+ bridgeGpio(std::move(bridgeGpio)), thresholdTimer(io)
{
sensorInterface = objectServer.add_interface(
"/xyz/openbmc_project/sensors/voltage/" + name,
@@ -88,6 +88,7 @@
// close the input dev to cancel async operations
inputDev.close();
waitTimer.cancel();
+
objServer.remove_interface(thresholdInterfaceWarning);
objServer.remove_interface(thresholdInterfaceCritical);
objServer.remove_interface(sensorInterface);
@@ -230,5 +231,5 @@
return;
}
- thresholds::checkThresholdsPowerDelay(this, thresholdTimer);
+ thresholds::checkThresholdsPowerDelay(weak_from_this(), thresholdTimer);
}
diff --git a/src/Thresholds.cpp b/src/Thresholds.cpp
index a74ec4a..bbe8e20 100644
--- a/src/Thresholds.cpp
+++ b/src/Thresholds.cpp
@@ -361,7 +361,8 @@
return thresholdChanges;
}
-void ThresholdTimer::startTimer(const Threshold& threshold, bool assert,
+void ThresholdTimer::startTimer(const std::weak_ptr<Sensor>& weakSensor,
+ const Threshold& threshold, bool assert,
double assertValue)
{
struct TimerUsed timerUsed = {};
@@ -386,8 +387,14 @@
pair->first.direction = threshold.direction;
pair->first.assert = assert;
pair->second.expires_from_now(boost::posix_time::seconds(waitTime));
- pair->second.async_wait([this, pair, threshold, assert,
+ pair->second.async_wait([weakSensor, pair, threshold, assert,
assertValue](boost::system::error_code ec) {
+ auto sensorPtr = weakSensor.lock();
+ if (!sensorPtr)
+ {
+ return; // owner sensor has been destructed
+ }
+ // pair is valid as long as sensor is valid
pair->first.used = false;
if (ec == boost::asio::error::operation_aborted)
@@ -399,9 +406,9 @@
std::cerr << "timer error: " << ec.message() << "\n";
return;
}
- if (sensor->readingStateGood())
+ if (sensorPtr->readingStateGood())
{
- assertThresholds(sensor, assertValue, threshold.level,
+ assertThresholds(sensorPtr.get(), assertValue, threshold.level,
threshold.direction, assert);
}
});
@@ -425,9 +432,16 @@
return status;
}
-void checkThresholdsPowerDelay(Sensor* sensor, ThresholdTimer& thresholdTimer)
+void checkThresholdsPowerDelay(const std::weak_ptr<Sensor>& weakSensor,
+ ThresholdTimer& thresholdTimer)
{
+ auto sensorPtr = weakSensor.lock();
+ if (!sensorPtr)
+ {
+ return; // sensor is destructed, should never be here
+ }
+ Sensor* sensor = sensorPtr.get();
std::vector<ChangeParam> changes = checkThresholds(sensor, sensor->value);
for (const auto& change : changes)
{
@@ -447,8 +461,8 @@
if (change.asserted || thresholdTimer.hasActiveTimer(
change.threshold, !change.asserted))
{
- thresholdTimer.startTimer(change.threshold, change.asserted,
- change.assertValue);
+ thresholdTimer.startTimer(weakSensor, change.threshold,
+ change.asserted, change.assertValue);
continue;
}
}