summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/mongo/db/default_baton.cpp7
-rw-r--r--src/mongo/db/operation_context.cpp96
-rw-r--r--src/mongo/db/operation_context.h19
-rw-r--r--src/mongo/db/service_context.cpp22
-rw-r--r--src/mongo/stdx/condition_variable.h13
-rw-r--r--src/mongo/transport/baton_asio_linux.h7
-rw-r--r--src/mongo/transport/transport_layer_asio.cpp9
7 files changed, 48 insertions, 125 deletions
diff --git a/src/mongo/db/default_baton.cpp b/src/mongo/db/default_baton.cpp
index 8aeef1bec64..1154d377ccb 100644
--- a/src/mongo/db/default_baton.cpp
+++ b/src/mongo/db/default_baton.cpp
@@ -57,11 +57,8 @@ void DefaultBaton::detachImpl() noexcept {
{
stdx::lock_guard<stdx::mutex> lk(_mutex);
- {
- stdx::lock_guard<Client> lk(*_opCtx->getClient());
- invariant(_opCtx->getBaton().get() == this);
- _opCtx->setBaton(nullptr);
- }
+ invariant(_opCtx->getBaton().get() == this);
+ _opCtx->setBaton(nullptr);
_opCtx = nullptr;
_hasIngressSocket = false;
diff --git a/src/mongo/db/operation_context.cpp b/src/mongo/db/operation_context.cpp
index 271ea547fa2..2e9e4c0a00e 100644
--- a/src/mongo/db/operation_context.cpp
+++ b/src/mongo/db/operation_context.cpp
@@ -246,49 +246,26 @@ Status OperationContext::checkForInterruptNoAssert() noexcept {
return Status::OK();
}
-// Theory of operation for waitForConditionOrInterruptNoAssertUntil and markKilled:
-//
-// An operation indicates to potential killers that it is waiting on a condition variable by setting
-// _waitMutex and _waitCV, while holding the lock on its parent Client. It then unlocks its Client,
-// unblocking any killers, which are required to have locked the Client before calling markKilled.
-//
-// When _waitMutex and _waitCV are set, killers must lock _waitMutex before setting the _killCode,
-// and must signal _waitCV before releasing _waitMutex. Unfortunately, they must lock _waitMutex
-// without holding a lock on Client to avoid a deadlock with callers of
-// waitForConditionOrInterruptNoAssertUntil(). So, in the event that _waitMutex is set, the killer
-// increments _numKillers, drops the Client lock, acquires _waitMutex and then re-acquires the
-// Client lock. We know that the Client, its OperationContext and _waitMutex will remain valid
-// during this period because the caller of waitForConditionOrInterruptNoAssertUntil will not return
-// while _numKillers > 0 and will not return until it has itself reacquired _waitMutex. Instead,
-// that caller will keep waiting on _waitCV until _numKillers drops to 0.
+
+// waitForConditionOrInterruptNoAssertUntil returns when:
//
-// In essence, when _waitMutex is set, _killCode is guarded by _waitMutex and _waitCV, but when
-// _waitMutex is not set, it is guarded by the Client spinlock. Changing _waitMutex is itself
-// guarded by the Client spinlock and _numKillers.
+// Normal condvar wait criteria:
+// - cv is notified
+// - deadline is passed
//
-// When _numKillers does drop to 0, the waiter will null out _waitMutex and _waitCV.
+// OperationContext kill criteria:
+// - _deadline is passed (artificial deadline or maxTimeMS)
+// - markKilled is called (killOp)
//
-// This implementation adds a minimum of two spinlock acquire-release pairs to every condition
-// variable wait.
+// Baton criteria:
+// - _baton is notified (someone's queuing work for the baton)
+// - _baton::run returns (timeout fired / networking is ready / socket disconnected)
StatusWith<stdx::cv_status> OperationContext::waitForConditionOrInterruptNoAssertUntil(
stdx::condition_variable& cv, stdx::unique_lock<stdx::mutex>& m, Date_t deadline) noexcept {
invariant(getClient());
- {
- stdx::lock_guard<Client> clientLock(*getClient());
- invariant(!_waitMutex);
- invariant(!_waitCV);
- invariant(_waitThread == kNoWaiterThread);
- invariant(0 == _numKillers);
-
- // This interrupt check must be done while holding the client lock, so as not to race with a
- // concurrent caller of markKilled.
- auto status = checkForInterruptNoAssert();
- if (!status.isOK()) {
- return status;
- }
- _waitMutex = m.mutex();
- _waitCV = &cv;
- _waitThread = stdx::this_thread::get_id();
+
+ if (auto status = checkForInterruptNoAssert(); !status.isOK()) {
+ return status;
}
// If the maxTimeNeverTimeOut failpoint is set, behave as though the operation's deadline does
@@ -314,22 +291,10 @@ StatusWith<stdx::cv_status> OperationContext::waitForConditionOrInterruptNoAsser
cv, m, deadline, _baton.get());
}();
- // Continue waiting on cv until no other thread is attempting to kill this one.
- Waitable::wait(_baton.get(), getServiceContext()->getPreciseClockSource(), cv, m, [this] {
- stdx::lock_guard<Client> clientLock(*getClient());
- if (0 == _numKillers) {
- _waitMutex = nullptr;
- _waitCV = nullptr;
- _waitThread = kNoWaiterThread;
- return true;
- }
- return false;
- });
-
- auto status = checkForInterruptNoAssert();
- if (!status.isOK()) {
+ if (auto status = checkForInterruptNoAssert(); !status.isOK()) {
return status;
}
+
if (opHasDeadline && waitStatus == stdx::cv_status::timeout && deadline == getDeadline()) {
// It's possible that the system clock used in stdx::condition_variable::wait_until
// is slightly ahead of the FastClock used in checkForInterrupt. In this case,
@@ -352,33 +317,8 @@ void OperationContext::markKilled(ErrorCodes::Error killCode) {
log() << "operation was interrupted because a client disconnected";
}
- stdx::unique_lock<stdx::mutex> lkWaitMutex;
-
- // If we have a _waitMutex, it means this opCtx is currently blocked in
- // waitForConditionOrInterrupt.
- //
- // From there, we also know which thread is actually doing that waiting (it's recorded in
- // _waitThread). If that thread isn't our thread, it's necessary to do the regular numKillers
- // song and dance mentioned in waitForConditionOrInterrupt.
- //
- // If it is our thread, we know that we're currently inside that call to
- // waitForConditionOrInterrupt and are being invoked by a callback run from Baton->run. And
- // that means we don't need to deal with the waitMutex and waitCV (because we're running
- // callbacks, which means run is returning, which means we'll be checking _killCode in the near
- // future).
- if (_waitMutex && stdx::this_thread::get_id() != _waitThread) {
- invariant(++_numKillers > 0);
- getClient()->unlock();
- ON_BLOCK_EXIT([this] {
- getClient()->lock();
- invariant(--_numKillers >= 0);
- });
- lkWaitMutex = stdx::unique_lock<stdx::mutex>{*_waitMutex};
- }
- _killCode.compareAndSwap(ErrorCodes::OK, killCode);
- if (lkWaitMutex && _numKillers == 0) {
- invariant(_waitCV);
- _waitCV->notify_all();
+ if (_killCode.compareAndSwap(ErrorCodes::OK, killCode) == ErrorCodes::OK) {
+ _baton->notify();
}
}
diff --git a/src/mongo/db/operation_context.h b/src/mongo/db/operation_context.h
index 56a363aab3e..ffa9678d7a0 100644
--- a/src/mongo/db/operation_context.h
+++ b/src/mongo/db/operation_context.h
@@ -446,27 +446,8 @@ private:
// once from OK to some kill code.
AtomicWord<ErrorCodes::Error> _killCode{ErrorCodes::OK};
- // A transport Baton associated with the operation. The presence of this object implies that a
- // client thread is doing it's own async networking by blocking on it's own thread.
BatonHandle _baton;
- // If non-null, _waitMutex and _waitCV are the (mutex, condition variable) pair that the
- // operation is currently waiting on inside a call to waitForConditionOrInterrupt...().
- //
- // _waitThread is the calling thread's thread id.
- //
- // All access guarded by the Client's lock.
- stdx::mutex* _waitMutex = nullptr;
- stdx::condition_variable* _waitCV = nullptr;
- stdx::thread::id _waitThread;
-
- // If _waitMutex and _waitCV are non-null, this is the number of threads in a call to markKilled
- // actively attempting to kill the operation. If this value is non-zero, the operation is inside
- // waitForConditionOrInterrupt...() and must stay there until _numKillers reaches 0.
- //
- // All access guarded by the Client's lock.
- int _numKillers = 0;
-
WriteConcernOptions _writeConcern;
// The timepoint at which this operation exceeds its time limit.
diff --git a/src/mongo/db/service_context.cpp b/src/mongo/db/service_context.cpp
index ee26756a642..ac8937dbc6c 100644
--- a/src/mongo/db/service_context.cpp
+++ b/src/mongo/db/service_context.cpp
@@ -248,21 +248,20 @@ ServiceContext::UniqueOperationContext ServiceContext::makeOperationContext(Clie
opCtx->setRecoveryUnit(std::make_unique<RecoveryUnitNoop>(),
WriteUnitOfWork::RecoveryUnitState::kNotInUnitOfWork);
}
- {
- stdx::lock_guard<Client> lk(*client);
- client->setOperationContext(opCtx.get());
- }
+ // The baton must be attached before attaching to a client
if (_transportLayer) {
_transportLayer->makeBaton(opCtx.get());
} else {
makeBaton(opCtx.get());
}
+ {
+ stdx::lock_guard<Client> lk(*client);
+ client->setOperationContext(opCtx.get());
+ }
return UniqueOperationContext(opCtx.release());
};
void ServiceContext::OperationContextDeleter::operator()(OperationContext* opCtx) const {
- opCtx->getBaton()->detach();
-
auto client = opCtx->getClient();
if (client->session()) {
_numCurrentOps.subtractAndFetch(1);
@@ -272,6 +271,8 @@ void ServiceContext::OperationContextDeleter::operator()(OperationContext* opCtx
stdx::lock_guard<Client> lk(*client);
client->resetOperationContext();
}
+ opCtx->getBaton()->detach();
+
onDestroy(opCtx, service->_clientObservers);
delete opCtx;
}
@@ -413,13 +414,10 @@ void ServiceContext::ServiceContextDeleter::operator()(ServiceContext* service)
}
BatonHandle ServiceContext::makeBaton(OperationContext* opCtx) const {
- auto baton = std::make_shared<DefaultBaton>(opCtx);
+ invariant(!opCtx->getBaton());
- {
- stdx::lock_guard<Client> lk(*opCtx->getClient());
- invariant(!opCtx->getBaton());
- opCtx->setBaton(baton);
- }
+ auto baton = std::make_shared<DefaultBaton>(opCtx);
+ opCtx->setBaton(baton);
return baton;
}
diff --git a/src/mongo/stdx/condition_variable.h b/src/mongo/stdx/condition_variable.h
index 33e42f927fa..031d5005484 100644
--- a/src/mongo/stdx/condition_variable.h
+++ b/src/mongo/stdx/condition_variable.h
@@ -47,6 +47,19 @@ namespace mongo {
*/
class Notifyable {
public:
+ // !!! PAY ATTENTION, THERE IS DANGER HERE !!!
+ //
+ // Implementers of the notifyable api must be level triggered by notify, rather than edge
+ // triggered.
+ //
+ // I.e. a call to notify() must either unblock the notifyable immediately, if it is currently
+ // blocked, or unblock it the next time it would wait, if it is not currently blocked.
+ //
+ // In addition to unblocking, the notifyable should also atomically consume that notification
+ // state as a result of waking. I.e. any number of calls to notify before or during a wait must
+ // unblock exactly one wait.
+ //
+ // Notifyable::notify is not like condition_variable::notify_X()
virtual void notify() noexcept = 0;
protected:
diff --git a/src/mongo/transport/baton_asio_linux.h b/src/mongo/transport/baton_asio_linux.h
index 1b6eb98db4f..c07ce5b32f4 100644
--- a/src/mongo/transport/baton_asio_linux.h
+++ b/src/mongo/transport/baton_asio_linux.h
@@ -394,11 +394,8 @@ private:
{
stdx::lock_guard<stdx::mutex> lk(_mutex);
- {
- stdx::lock_guard<Client> lk(*_opCtx->getClient());
- invariant(_opCtx->getBaton().get() == this);
- _opCtx->setBaton(nullptr);
- }
+ invariant(_opCtx->getBaton().get() == this);
+ _opCtx->setBaton(nullptr);
_opCtx = nullptr;
diff --git a/src/mongo/transport/transport_layer_asio.cpp b/src/mongo/transport/transport_layer_asio.cpp
index 8843478a4ee..dc0475b801d 100644
--- a/src/mongo/transport/transport_layer_asio.cpp
+++ b/src/mongo/transport/transport_layer_asio.cpp
@@ -879,13 +879,10 @@ SSLParams::SSLModes TransportLayerASIO::_sslMode() const {
#ifdef __linux__
BatonHandle TransportLayerASIO::makeBaton(OperationContext* opCtx) const {
- auto baton = std::make_shared<BatonASIO>(opCtx);
+ invariant(!opCtx->getBaton());
- {
- stdx::lock_guard<Client> lk(*opCtx->getClient());
- invariant(!opCtx->getBaton());
- opCtx->setBaton(baton);
- }
+ auto baton = std::make_shared<BatonASIO>(opCtx);
+ opCtx->setBaton(baton);
return baton;
}
View Lorry Controller Status