aws / aws/aws-sdk-cpp

Liveness / performance bug in PooledThreadExecutor

Offen
#2,744 3 Kommentare 0 Reaktionen 0 zugewiesene Personen Beansprucht von @jmklix Auf GitHub ansehen
bug p2 pending-release
Vorherrschende Sprache
C++
Sterne
2.2k
Forks
1.2k
Ø Merge
4 T. 11 Std.
Gemergte PRs (30 T.)
13

Beschreibung

### Describe the bug

My team noticed a performance / "liveness" bug when using the PooledThreadExecutor to handle lots of requests to S3. Our repro is reliable, but unfortunately too complex include with this report, so a description will have to suffice.

We observed lots of "producer" threads waiting for the SDK to complete requests that they had added to the PooledThreadExecutor's queue, while almost all of the PooledThreadExecutor's "consumer" ThreadTasks were idle, waiting [at this line](https://github.com/aws/aws-sdk-cpp/blob/50401cd2c87eb32e13e5acc1e0b11ce0edf2b088/src/aws-cpp-sdk-core/source/utils/threading/ThreadTask.cpp#L38).

The simplest description of the bug is that function `ThreadTask::MainTaskRunner()` doesn't use or hold the same mutex, between checking `m_executor.HasTasks()` and when it waits on `m_executor.m_sync.WaitOne()`. This allows a concurrent "producer" thread to add a task to the queue, after `ThreadTask::MainTaskRunner()` has concluded that the queue is empty, but before `ThreadTask::MainTaskRunner()` waits on the `Semaphore`.

In such a case, the queue has a task to be performed, but the `ThreadTask` does not realize this.

### Expected Behavior

When pushing a task to the PooledThreadExecutor's queue, if a `ThreadTask` is available, then it should execute that task.

### Current Behavior

PooledThreadExecutor's queue is non-empty, but `ThreadTask`s remain idle.

In my team's reopro, overall throughput drops dramatically, with hundreds of threads all waiting, ultimately, on the PooledThreadExecutor's Semaphore. (Eventually, in our repro, the "producer" threads fill their queues, so they end up blocked on the `ThreadTask`s, which are not executing the work on the PooledThreadExecutor's queue.)

### Reproduction Steps

Not feasible to repro at scale, here.

### Possible Solution

Function `ThreadTask::MainTaskRunner()` needs to hold the mutex until it waits on the `Semaphore`. Unfortunately, the SDK's `Semaphore` class doesn't take a mutex parameter, so the function would probably have to use `std::condition_variable`, instead. (Note that this code already uses `std::lock_guard` and `std::mutex`, so also using `std::unique_lock` and `std::condition_variable` should not be a problem.)

Possible solution, something like this (with corresponding changes made elsewhere):
```
void ThreadTask::MainTaskRunner()
{
while (m_continue)
{
std::unique_lock lock(m_executor.m_queueLock);
while (m_continue && m_executor.HasTasks() /*must not acquire m_queueLock anymore*/)
{
auto fn = m_executor.PopTask();
lock.unlock();
if(fn)
{
(*fn)();
Aws::Delete(fn);
}
lock.lock();
}

if(m_continue)
{
m_executor.m_cv.wait(lock, ...);
}
}
}
```

### Additional Information/Context

_No response_

### AWS CPP SDK version used

1.11.x

### Compiler and Version used

gcc 8

### Operating System and version

Linux 5.4.258

Beitragsleitfaden

Beitragsleitfaden öffnen

Rechercherichtung

Beginne bei src/aws-cpp-sdk-core/source/utils/threading/ThreadTask.cpp, insbesondere bei ThreadTask::MainTaskRunner(), und untersuche die Warteschlange von PooledThreadExecutor sowie die Synchronisierung mit Semaphore. Prüfe den verknüpften pull request #3040, bevor du den Umfang festlegst. Als erledigt gilt, dass eine eingereihtе Aufgabe zwischen der Prüfung auf eine leere Warteschlange und dem Warten nicht übersehen werden kann und verfügbare ThreadTasks unter gleichzeitig produzierenden Threads die Arbeit wieder aufnehmen.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Bewertung

Tech-Stack
aws, cpp
Bereich
backend
Issue-Typ
Bug
Schwierigkeit
4/5
Geschätzter Aufwand
3-5 Tage
Aktivitätsstatus
Veraltet
Klarheit
Größtenteils klar
Anfängerfreundlichkeit
25/100

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.