aws / aws/aws-sdk-cpp

Liveness / performance bug in PooledThreadExecutor

Aperta
#2,744 3 commenti 0 reazioni 0 assegnatari Rivendicata da @jmklix Vedi su GitHub
bug p2 pending-release
Lingua principale
C++
Stelle
2.2k
Fork
1.2k
Merge medio
4g 11h
PR unite (30g)
13

Descrizione

### 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

Guida per i contributori

Apri la guida per i contributori

Direzione di ricerca

Inizia da src/aws-cpp-sdk-core/source/utils/threading/ThreadTask.cpp, in particolare da ThreadTask::MainTaskRunner(), e ispeziona la coda di PooledThreadExecutor e la sincronizzazione di Semaphore. Esamina il pull request #3040 collegato prima di decidere l'ambito. Il lavoro è completato quando un'attività accodata non può essere persa tra il controllo della coda vuota e l'attesa e i ThreadTasks disponibili riprendono il lavoro in presenza di produttori concorrenti.

Scritto dal modello di indicizzazione a partire dal testo della issue.

Valutazione

Stack tecnologico
aws, cpp
Ambito
backend
Tipo di issue
Bug
Difficoltà
4/5
Tempo stimato
3-5 giorni
Stato di attività
Ferma
Chiarezza
Abbastanza chiara
Idoneità per principianti
25/100

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.