Skip to content

Commit 6bfbdab

Browse files
andrei-pavelandrei-pavel
committed
[#3025] fix ProcessSpawn on BSD
- Always break after collecting exit status. Previously it broke the loop always on failure of waitpid which does happen after calling it subsequently, but there is no reason to wait until then. - When waitpid returns -1 in sync mode, throw exception, except for EINTR which happens on signals (usually one time) prior to the child process exiting if sigaction is called without SA_RESTART which is the default on some systems. - Only initialize the global IO signal set on the IO service in async mode. It makes no sense to do it in sync mode because there is no IO service. - Swap pid and wpid names to conform to names in `man wait` on BSD. - Add FAIL() on timer expiration. - Don't call runOne() the third time in unit tests because it waits for the timer to expire.
1 parent a91ebc6 commit 6bfbdab

File tree

2 files changed

+45
-34
lines changed

2 files changed

+45
-34
lines changed

‎src/lib/asiolink/process_spawn.cc

+36-15
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@ class ProcessSpawnImpl : boost::noncopyable {
209209
/// for any child process
210210
/// @param sync whether this function is called immediately after spawning
211211
/// (synchronous) or not (asynchronous, default).
212-
static void waitForProcess(int signum, pid_t const pid = -1, bool const sync = false);
212+
static void waitForProcess(int signum, pid_t const wpid = -1, bool const sync = false);
213213

214214
/// @brief A map holding the status codes of executed processes.
215215
static ProcessCollection process_collection_;
@@ -332,7 +332,9 @@ ProcessSpawnImpl::getCommandLine() const {
332332
pid_t
333333
ProcessSpawnImpl::spawn(bool dismiss) {
334334
lock_guard<std::mutex> lk(mutex_);
335-
ProcessSpawnImpl::IOSignalSetInitializer::initIOSignalSet(io_service_);
335+
if (!sync_) {
336+
ProcessSpawnImpl::IOSignalSetInitializer::initIOSignalSet(io_service_);
337+
}
336338
// Create the child
337339
pid_t pid = fork();
338340
if (pid < 0) {
@@ -413,7 +415,7 @@ ProcessSpawnImpl::allocateInternal(const std::string& src) {
413415

414416
void
415417
ProcessSpawnImpl::waitForProcess(int /* signum */,
416-
pid_t const pid /* = -1 */,
418+
pid_t const wpid /* = -1 */,
417419
bool sync /* = false */) {
418420
// In synchronous mode, the lock is taken by the caller function
419421
// spawn(), so do not deadlock.
@@ -427,18 +429,37 @@ ProcessSpawnImpl::waitForProcess(int /* signum */,
427429
// When called asynchronously, the first IO context event is for
428430
// receiving the SIGCHLD signal which itself is blocking,
429431
// hence no need to block here too.
430-
pid_t wpid = waitpid(pid, &status, sync ? 0 : WNOHANG);
431-
if (wpid <= 0) {
432-
break;
433-
}
434-
for (auto const& instance : process_collection_) {
435-
auto const& proc = instance.second.find(wpid);
436-
/// Check that the terminating process was started
437-
/// by our instance of ProcessSpawn
438-
if (proc != instance.second.end()) {
439-
// In this order please
440-
proc->second->status_ = status;
441-
proc->second->running_ = false;
432+
pid_t pid = waitpid(wpid, &status, sync ? 0 : WNOHANG);
433+
if (pid < 0) {
434+
if (!sync) {
435+
break;
436+
}
437+
if (errno == EINTR) {
438+
// On some systems that call sigaction wihout SA_RESTART by default, signals make
439+
// waitpid exit with EINTR. In this situation, if sync mode is enabled, we're
440+
// interested in another round of waitpid.
441+
continue;
442+
}
443+
isc_throw(InvalidOperation, "process with pid " << wpid << " has returned " << pid
444+
<< " from waitpid in sync mode, errno: "
445+
<< strerror(errno));
446+
} else if (pid == 0) {
447+
if (!sync) {
448+
break;
449+
}
450+
} else /* if (pid > 0) */ {
451+
for (auto const& instance : process_collection_) {
452+
auto const& proc = instance.second.find(pid);
453+
/// Check that the terminating process was started
454+
/// by our instance of ProcessSpawn
455+
if (proc != instance.second.end()) {
456+
proc->second->status_ = status;
457+
proc->second->running_ = false;
458+
}
459+
}
460+
if (sync) {
461+
// Collected process status. Can exit loop.
462+
break;
442463
}
443464
}
444465
}

‎src/lib/asiolink/tests/process_spawn_unittest.cc

+9-19
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,11 @@ class ProcessSpawnTest : public ::testing::Test {
6868
io_signal_set_->remove(SIGCHLD);
6969
io_signal_set_.reset();
7070
// Make sure the cancel handler for the IOSignalSet is called.
71-
io_service_->poll();
71+
io_service_->restart();
72+
try {
73+
io_service_->poll();
74+
} catch (...) {
75+
}
7276
}
7377

7478
/// @brief Method used as the IOSignalSet handler.
@@ -98,6 +102,7 @@ class ProcessSpawnTest : public ::testing::Test {
98102
/// @brief Failsafe timer expiration handler.
99103
void testTimerHandler() {
100104
io_service_->stop();
105+
FAIL() << "Test Time: " << test_time_ms_ << " expired";
101106
}
102107
};
103108

@@ -119,8 +124,6 @@ TEST_F(ProcessSpawnTest, spawnWithArgs) {
119124
io_service_->runOne();
120125

121126
// waitForProcess handler.
122-
io_service_->runOne();
123-
124127
// ProcessSpawnTest::processSignal handler.
125128
io_service_->runOne();
126129

@@ -155,8 +158,6 @@ TEST_F(ProcessSpawnTest, spawnWithArgsAndEnvVars) {
155158
io_service_->runOne();
156159

157160
// waitForProcess handler.
158-
io_service_->runOne();
159-
160161
// ProcessSpawnTest::processSignal handler.
161162
io_service_->runOne();
162163

@@ -189,14 +190,15 @@ TEST_F(ProcessSpawnTest, spawnTwoProcesses) {
189190
io_service_->runOne();
190191

191192
// waitForProcess handler.
192-
io_service_->runOne();
193-
194193
// ProcessSpawnTest::processSignal handler.
195194
io_service_->runOne();
196195

197196
// Poll to be sure.
198197
io_service_->poll();
199198

199+
ASSERT_EQ(1, processed_signals_.size());
200+
ASSERT_EQ(SIGCHLD, processed_signals_[0]);
201+
200202
pid_t pid2 = 0;
201203
ASSERT_NO_THROW(pid2 = process.spawn());
202204

@@ -207,8 +209,6 @@ TEST_F(ProcessSpawnTest, spawnTwoProcesses) {
207209
io_service_->runOne();
208210

209211
// waitForProcess handler.
210-
io_service_->runOne();
211-
212212
// ProcessSpawnTest::processSignal handler.
213213
io_service_->runOne();
214214

@@ -246,8 +246,6 @@ TEST_F(ProcessSpawnTest, spawnNoArgs) {
246246
io_service_->runOne();
247247

248248
// waitForProcess handler.
249-
io_service_->runOne();
250-
251249
// ProcessSpawnTest::processSignal handler.
252250
io_service_->runOne();
253251

@@ -269,8 +267,6 @@ TEST_F(ProcessSpawnTest, spawnNoArgs) {
269267
io_service_->runOne();
270268

271269
// waitForProcess handler.
272-
io_service_->runOne();
273-
274270
// ProcessSpawnTest::processSignal handler.
275271
io_service_->runOne();
276272

@@ -347,8 +343,6 @@ TEST_F(ProcessSpawnTest, isRunning) {
347343
io_service_->runOne();
348344

349345
// waitForProcess handler.
350-
io_service_->runOne();
351-
352346
// ProcessSpawnTest::processSignal handler.
353347
io_service_->runOne();
354348

@@ -379,8 +373,6 @@ TEST_F(ProcessSpawnTest, inheritEnv) {
379373
io_service_->runOne();
380374

381375
// waitForProcess handler.
382-
io_service_->runOne();
383-
384376
// ProcessSpawnTest::processSignal handler.
385377
io_service_->runOne();
386378

@@ -415,8 +407,6 @@ TEST_F(ProcessSpawnTest, inheritEnvWithParentVar) {
415407
io_service_->runOne();
416408

417409
// waitForProcess handler.
418-
io_service_->runOne();
419-
420410
// ProcessSpawnTest::processSignal handler.
421411
io_service_->runOne();
422412

0 commit comments

Comments
 (0)